-
Notifications
You must be signed in to change notification settings - Fork 936
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
Add description to some parameters #4716
Conversation
d193d20
to
0de933d
Compare
.UseStandardRelationalOperatorParameters("number"); | ||
.UseStandardRelationalOperatorParameters( | ||
"number", gd::ParameterOptions::MakeNewOptions()); |
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.
This should probably be the default parameter value.
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.
When talking about this with Alex, I was worried of default values in JS. In C++ I guess it could work, and I guess if we put the "optional" flag in Bindings.idl, Emscripten would generate a call with the default value (well, a call to the function without the parameter, so it would take the default value).
I think we should:
- Still keep the MakeNewOptions for safety for now and consistency.
- Add the default parameter so that we can then remove things to make them less verbose.
.UseStandardOperatorParameters( | ||
"number", | ||
ParameterOptions::MakeNewOptions().SetDescription( | ||
_("Volume (0-100)"))) |
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 wonder if the method could be directly in the metadata to make the declaration faster to write and easier to read.
What do you think about renaming UseStandardOperatorParameters
to something like AddOperatorAndValueParameters
and declaring methods like SetLastParameterDescription
?
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 was thinking initially of using SetLastParameterDescription, but I have the feeling that it lacks some robustness... what if, one day for some reason, the value is not the "last parameter" anymore?
What if the parameter options include something like an acceptable range, that would have an impact on what operator is valid to use?
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.
Lgtm. As discussed, this ParameterOptions might be removed entirely in the future in favor of a ValueTypeMetadata that would contain more information about a type (type, "extra info", acceptable value, unit - at least :)). It could help represents:
number (without units), string, but also things like "degrees", "speed" (number with a unit) or even "volume" (number with a unit and an acceptable range), "string with selector" (string with a list of acceptable options), etc...
Fixes #4701
When using the utils
UseStandardRelationalOperatorParameters
,UseStandardOperatorParameters
andUseStandardParameters
, this PR adds a new objectParameterOptions
that can specify the parameterdescription
andtypeExtraInfo
(used for selectors). It might holds units in the future.This allows to replace generic descriptions
Value
andValue to compare
with things like: