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

Fix #105, allow trailing commas in query macros #109

Merged
merged 3 commits into from
Feb 21, 2020

Conversation

tim-harding
Copy link
Contributor

@tim-harding tim-harding commented Feb 8, 2020

Fixes #105

@tim-harding tim-harding changed the title Fix #104, allow trailing commas in query macros Fix #105, allow trailing commas in query macros Feb 9, 2020
@tim-harding tim-harding force-pushed the ab/trailing-comma-fix branch from 487d45e to cefd377 Compare February 9, 2020 00:17
@abonander
Copy link
Collaborator

abonander commented Feb 9, 2020

Looking good! I'd like to have this tested for query_file!() and query_file_as!() as well; I think we're fine with the doctests covering the case of no-trailing-comma.

@tim-harding
Copy link
Contributor Author

tim-harding commented Feb 10, 2020

Sounds good. I was also curious about your comment on another thread about not using $(,)? because it introduces a metavariable. Are there compiler performance downsides to unused metavariables or some other reason you don't prefer that approach?

@abonander
Copy link
Collaborator

Okay, so $(,)? does work in the matcher for parsing; the mistake I made was for some reason I wanted to pass that trailing comma to the inner macro since it may accept them, but besides the fact that would be completely pointless, there's no way to capture that extra comma in the expansion because it's not bound to a metavariable. Basically I just had a brainfart. You can absolutely just use $(,)? to permit an optional trailing comma without having to introduce an extra matcher to the macro.

@abonander
Copy link
Collaborator

abonander commented Feb 19, 2020

@timmythetiny did you want to do the above simplification? Sorry for overcomplicating things before.

@tim-harding
Copy link
Contributor Author

I'll get to it tonight. Sorry for the delay.

@abonander
Copy link
Collaborator

No worries, I just wanted to make sure you were still interested. I was tempted to just do it myself, but I would have merged this anyway so you got the contributor credit.

Merged third arm into second arm with a metavariable
@tim-harding
Copy link
Contributor Author

Reasonable enough. It was a very easy change.

@abonander abonander merged commit 18a925f into launchbadge:master Feb 21, 2020
@abonander
Copy link
Collaborator

Perfect!

@tim-harding tim-harding deleted the ab/trailing-comma-fix branch February 21, 2020 06:41
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.

query! macros should allow for a trailing comma
2 participants