-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
tokio-trace: Extend macros to allow trailing commas #891
tokio-trace: Extend macros to allow trailing commas #891
Conversation
Trailing commas are optional when fields are included, and when fields are delimited from the format string for the message. Signed-off-by: Kevin Leimkuhler <kevinl@buoyant.io>
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 LGTM, and I'm happy to merge as-is.
For extra credit, I'd love it if you added examples of events with trailing commas to the RustDocs for these macros, but it's not a big deal.
@@ -9,97 +9,121 @@ extern crate tokio_trace; | |||
#[test] | |||
fn event() { | |||
event!(tokio_trace::Level::DEBUG, foo = 3, bar = 2, baz = false); | |||
event!(tokio_trace::Level::DEBUG, foo = 3, bar = 3,); |
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.
Thanks for adding to the tests, this is great!
@kleimkuhler can we also make a similar change for the |
@hawkw I'm not sure I like how this currently is. An unlimited number of trailing commas are valid anywhere - not just the last field.
is valid. Let me clean this up to allow only 1 trailing comma on the final field. |
After some investigation, this is an issue that is fixed by the Kleene operator being stabilized as a macro repetition operator. Unfortunately, this RFC was merge and is only stable on As mentioned here, the best way to validate a trailing comma is introducing an identical rule that has a trailing comma. I will follow this up with those changes. This essentially doubles our rules for each macro, so after it is ready to go we can evaluate if that is something we want to introduce. It makes the macros look cluttered. |
This change also adds trailing commas to `span!` Doc tests and notes on the validity in comments. Signed-off-by: Kevin Leimkuhler <kevinl@buoyant.io>
f3939e6
to
54a1bfe
Compare
Signed-off-by: Kevin Leimkuhler <kevinl@buoyant.io>
@kleimkuhler thanks, this is great --- I've merged now that the CI build has passed. |
Trailing commas are optional when fields are included, and when fields
are delimited from the format string for the message.
Before these would error:
Now they do not. Tests have been extended.
Signed-off-by: Kevin Leimkuhler kevinl@buoyant.io