-
Notifications
You must be signed in to change notification settings - Fork 30
AOTF: Do not send non-required mutation args when they have the default value #2230
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
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.
Makes sense, but you've got some tests to fix.
…lt value This workaround helps avoid back-compat issues when we make changes to the schema
cbae1d7
to
ce0aaf9
Compare
let pointer = arg.type | ||
let multiple = false | ||
let cylcObject = null | ||
let cylcType = null | ||
const required = arg.type?.kind === 'NON_NULL' |
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.
Jslint used to tell us not to declare variables within loops like this. More performant to declare once and reuse?
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.
I can't find much info on that and perhaps that dated back to when var
was used to declare variables?
I prefer this way because a) it reduces duplication and makes the code more readable, b) it is less likely to lead to the value from one iteration accidentally leaking into the next iteration - the scope is only within a single iteration.
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.
dated back to when var was used to declare variables?
That's probably it. Using var
within a loop would re-declare the variable over and over.
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.
Actually the opposite I think var
declarations were hoisted 😬
As mentioned in cylc/cylc-flow#6820, the reload command in the GUI currently will be broken for workflows running in Cylc < 8.5.0.
This is because cylc/cylc-flow#6509 added a new argument in the schema. However we can work around this by not sending that arg when it is the default value.
This workaround can be removed when #1225 is addressed.
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.?.?.x
branch.