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

Refactor format_args! expansion parsing #7442

Merged
merged 5 commits into from
Jul 13, 2021
Merged

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Jul 7, 2021

Introduces FormatExpn::parse and FormatArgsExpn::parse. Motivated by rust-lang/rust#83302, so I only have to change Clippy in one place. Fixed an FP along the way.

I also allowed needless_bool in macros because I often want to do if_chain! { .. then { true } else { false } }.

changelog: Fix false positive in useless_format when some text is appended or prepended to a single string with some useless formatting params
changelog: Allow needless_bool in macros

@flip1995
Copy link
Member

flip1995 commented Jul 8, 2021

r? @xFrednet Can you take over this review please? (rust-highfive was down, so I'll give the 2 unassigned PRs to you)

@flip1995 flip1995 self-assigned this Jul 8, 2021
@xFrednet
Copy link
Member

xFrednet commented Jul 8, 2021

Sure, I'm happy to take them over! 🙃

@camsteffen camsteffen assigned xFrednet and unassigned flip1995 Jul 8, 2021
@xFrednet
Copy link
Member

xFrednet commented Jul 8, 2021

^^ thank you! I believe that @flip1995 would also like to stay assigned to double-check my review later. 🙃

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, awesome work, it took some time digging though it all but the result is worth it. Thank you for the nice refactoring!

clippy_lints/src/explicit_write.rs still contains some lint messages with punctuation and a suggestion in the main message (lines 111ff). Could you update the lint emission to display the help message using span_lint_and_help while you're in the area?

clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
clippy_lints/src/format.rs Outdated Show resolved Hide resolved
@camsteffen
Copy link
Contributor Author

Thanks for the thorough review @xFrednet. I addressed your comments.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this looks good to me 👍

Hey @flip1995, I'm done with my review and would hand this over to you now. If you weren't taking over, I would ask @camsteffen to squash his fixup commits before merging it. 🙃

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xFrednet Thanks for doing the review!

@camsteffen please squash the fixup commits and then r=xFrednet 👍

@camsteffen
Copy link
Contributor Author

@bors r=xFrednet

@bors
Copy link
Collaborator

bors commented Jul 13, 2021

📌 Commit 306f9e8 has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented Jul 13, 2021

⌛ Testing commit 306f9e8 with merge bf4512e...

@bors
Copy link
Collaborator

bors commented Jul 13, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing bf4512e to master...

@bors bors merged commit bf4512e into rust-lang:master Jul 13, 2021
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.

4 participants