-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: extensions_options
macro
#5442
feat: extensions_options
macro
#5442
Conversation
a175acf
to
3ba4d39
Compare
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.
Looks good to me!
The default value might be better with an attribute macro, but it would be much more complicated. This one looks pretty enough 👍
The issue with prog macros (which I think is what you mean with "attribute macros") is: they are slower, more complicated to write & test, and you ideally have a trait just for the prog macro because they are compiled just to check/document the downstream and you wanna avoid that for costly crates. If you meant "can you just use a macro syntax for defaults"? Ahh, maybe. If there's strong desire to do that, I can try (although I'm not sure if traditional macros can match on macro-like attributes). |
Agree, I tried one recently and it does take me lots of time to debug 😢
As long as the current implementation works fine and does not have ambiguous grammar, we can keep using it. And to me it's straitforward 👍 |
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.
Other than the location of the test I think this one is ready to go -- thank you @crepererum
For anyone following along, the config was added to IOx in https://github.com/influxdata/influxdb_iox/pull/7020
3ba4d39
to
e6ae53e
Compare
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.
Thank you @crepererum -- this is really nice. Thank you @waynexia for the review
fn main() { | ||
// set up config struct and register extension | ||
let mut config = ConfigOptions::default(); | ||
config.extensions.insert(MyConfig::default()); |
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.
❤️
@@ -634,3 +634,129 @@ trait Visit { | |||
|
|||
fn none(&mut self, key: &str, description: &'static str); | |||
} | |||
|
|||
/// Convenience macro to create [`ExtensionsOptions`]. |
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.
Benchmark runs are scheduled for baseline = ac618f0 and contender = e985207. e985207 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
-
Rationale for this change
I was implementing
ExtensionsOptions
in InfluxDB IOx and it was quite cumbersome, since you need to be very careful to keep all theset
/entries
methods in sync with the structure definition. So I wrote a macro that does a similar magic than the DF builtin configs. I would like to upstream this since I think it actually makes config extensions usable.What changes are included in this PR?
A new macro.
Are these changes tested?
Small "does it expand" test.
Are there any user-facing changes?
New macro.