Skip to content
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

Conversation

kleimkuhler
Copy link
Contributor

Trailing commas are optional when fields are included, and when fields
are delimited from the format string for the message.

Before these would error:

event!(tokio_trace::Level::DEBUG, foo = 3, bar = 3,);
event!(tokio_trace::Level::DEBUG, { foo = 2, bar = 78, }, "baz");

Now they do not. Tests have been extended.

Signed-off-by: Kevin Leimkuhler kevinl@buoyant.io

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>
@hawkw hawkw requested review from hawkw and davidbarsky February 11, 2019 21:45
Copy link
Member

@hawkw hawkw left a 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.

tokio-trace/src/lib.rs Outdated Show resolved Hide resolved
@@ -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,);
Copy link
Member

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!

@hawkw
Copy link
Member

hawkw commented Feb 11, 2019

@kleimkuhler can we also make a similar change for the span! macro?

@kleimkuhler
Copy link
Contributor Author

@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.

event!(
   tokio_trace::Level::DEBUG, 
   foo = 1,,,,,,,,
   baz = 2,
   ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
);

is valid. Let me clean this up to allow only 1 trailing comma on the final field.

@kleimkuhler
Copy link
Contributor Author

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 1.32 with edition = "2018".

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>
@kleimkuhler kleimkuhler force-pushed the kleimkuhler/tokio-trace-extend-macros branch from f3939e6 to 54a1bfe Compare February 11, 2019 23:27
Signed-off-by: Kevin Leimkuhler <kevinl@buoyant.io>
@hawkw hawkw merged commit 93e66e6 into tokio-rs:eliza/tokio-trace Feb 12, 2019
@hawkw
Copy link
Member

hawkw commented Feb 12, 2019

@kleimkuhler thanks, this is great --- I've merged now that the CI build has passed.

@kleimkuhler kleimkuhler deleted the kleimkuhler/tokio-trace-extend-macros branch February 12, 2019 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants