-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Error parameter not popped up prompt during variable splitting and parameter extraction node verification #4238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…nd parameter extraction node verification
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| }) | ||
| } | ||
| onMounted(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no irregularities or significant issues with the provided code snippet. The code appears to be correctly structured and contains typical practices for form validation using El-form from Element UI in Vue.js.
Suggested Optimization:
1. Error Handling in validate Function:
- Although there's already error handling in this part of the code using
.catch(), it could benefit from adding a more clear message when an error occurs. This can help debugging and improve user experience if something goes wrong.
try:
await VariableSplittingRef.value.validate()
except ValidationErrors as e:
throw new Error(`Validation failed for variable spli<|fim_suffix|>on will then catch and reject this promise with additional data about the validation errors, including details such as the affected node and the specific error messages. If successful, the validated result is returned normally.| }) | ||
| } | ||
| onMounted(() => { | ||
| set(props.nodeModel, 'validate', validate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two main issues in this code:
-
The
messagestring in the validation rule is not localized correctly when using internationalization library$t. -
There's an error handling block inside
validate()where it catches rejection, but it incorrectly returns{ node: props.nodeModel, errMessage: err }. This might prevent further actions from being processed correctly.
Suggested Changes
const rules = computed(() => ({
- variable_list: [
+ variable_list: {
required: true,
message: $t(
'views.applicationWorkflow.nodes.variableSplittingNode.variableListPlaceholder',
),
- trigger: 'blur'
+ },
+ trigger: ['blur', 'change'],
}),
}))Explanation of changes:
- Added braces to make the
messageproperty inline with other properties of the same level, which makes object syntax cleaner. - Enhanced the trigger to include both blur and change events (
trigger: ['blur', 'change']) to ensure that the form item checks if all required fields have been filled without losing focus after changing values.
Regarding the second issue of returning the promise directly instead of passing it through reject (which would require additional code elsewhere to handle), I think the correct approach is to either propagate errors properly so they can be handled higher up in your application logic, or to log them appropriately within the catch block if there's no possibility of handling them further. If you're confident these errors won't need to be propagated, leaving them caught should be sufficient for debugging purposes. But usually, it would best practice to fix what caused the initial failure rather than ignore it silently.
| return {variable['field']: {'type': variable['parameter_type'], 'description': (variable.get('desc') or ""), | ||
| 'title': variable['label']} for variable in | ||
| variable_list} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided code appears to be generating property settings based on a list of variables with fields like field, parameter_type, desc, and label. The main issue is that if there's no description ('desc') provided for a variable, it currently raises an error because you're trying to access None properties directly.
I suggest:
- Replace the line:
'type': variable['type'], 'description': (variable.get('desc') or ""), - With this one:
'type': variable['type'], 'description': str((variable.get('desc', '')),
This modification ensures that if variable['desc'] is None, it defaults to an empty string, avoiding any runtime errors.
fix: Error parameter not popped up prompt during variable splitting and parameter extraction node verification