-
Notifications
You must be signed in to change notification settings - Fork 90
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
Insert Options attributes with initializer_list #2598
Conversation
This is probably more readable if setting multiple attributes: ``` Options options; options["value"].insertAttributes({ {"units", "m/s"}, {"conversion", 10.2}, {"long_name", "important value"} }); ``` Uses `std::map::emplace` internally, so doesn't replace or update values. Includes tests and some documentation.
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.
clang-tidy made some suggestions
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! I wonder if we should go even further and have something like:
namespace bout {
struct Metadata {
std::string units;
std::string conversion;
std::string long_name;
}
}
Maybe that actually adds too much complication for not enough benefit.
Uses
std::map::emplace
internally, so doesn't replace or update values.
It would be good to note this in the docstring and the manual, might be surprising otherwise!
Change the semantics so that attributes are always set, rather than not replacing already set attributes. This way is probably more intuitive.
…-dev into next-options-attributes
Thanks @ZedThree ! The semantics of |
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.
clang-tidy made some suggestions
clang-tidy review says "All clean, LGTM! 👍" |
Forgot to update the test, now that attributes are replaced.
clang-tidy review says "All clean, LGTM! 👍" |
This is probably more readable if setting multiple attributes:
Usesstd::map::emplace
internally, so doesn't replace or update values.Replaces any values that were previously set.
Includes tests and some documentation.