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

Move format_push_string to restriction #9161

Merged

Conversation

Victor-N-Suadicani
Copy link
Contributor

@Victor-N-Suadicani Victor-N-Suadicani commented Jul 12, 2022

Fixes #9077 (kinda) by moving the lint to the restriction group. As I noted in that issue, I think the suggested change is too much and as the OP of the issue points out, the ramifications of the change are not necessarily easily understood. As such I don't think the lint should be enabled by default.

changelog: [format_push_string]: moved to restriction (see #9077).

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 12, 2022
@yoav-lavi
Copy link
Contributor

yoav-lavi commented Jul 12, 2022

Hey, author of #9077.

Thank you!

If we want to move the lint, I'd like to note that using clippy::pedantic is (anecdotally) popular from what I've seen, so perhaps clippy::restrictions (or clippy::nursery)?

Considering that the issues with the lint don't really fall into "occasionally a false positive" or "too strict", I personally think it falls more into the category of a restriction

(Although to be fair, I use clippy::pedantic so I may be biased 🙂)

@Victor-N-Suadicani Victor-N-Suadicani changed the title Move format_push_string to pedantic Move format_push_string to restriction Jul 13, 2022
@Victor-N-Suadicani
Copy link
Contributor Author

I totally forgot that restriction even existed and I agree that pedantic wasn't the best fit. I've moved it to restriction now :)

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.

Thanks! I agree with the reasoning in the linked issue and that it should be moved to restriction. Can you please add a summary of this issue to a new ### Known Issues section in the link documentation after the why is this bad section? This should just summarize, that changing the code to write will lead to usage of a fallible API.

@Jarcho
Copy link
Contributor

Jarcho commented Jul 13, 2022

Just going to add a couple points here.

  • format! can fail for exactly the same reason write! to a string can fail. It just calls expect internally.
  • The only way this can return an error is if a Display implementation creates the error. This would be a violation of the stated API

I see this as a combination of the lint docs not being enough to justify the change, and an unfortunate side-effect of having to make the unwrap call explicit.

@yoav-lavi
Copy link
Contributor

yoav-lavi commented Jul 13, 2022

@Jarcho

format! can fail for exactly the same reason write! to a string can fail. It just calls expect internally.

That was my assumption, the lint (in my opinion) doesn't make that clear enough (with the added issue of encouraging ignoring an error ad @Victor-N-Suadicani stated in the issue)

@Victor-N-Suadicani
Copy link
Contributor Author

@flip1995 Added the known problems section :)

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Jul 14, 2022

📌 Commit 43ac256 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 14, 2022

⌛ Testing commit 43ac256 with merge 10e85ef...

@bors
Copy link
Collaborator

bors commented Jul 14, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 10e85ef to master...

@bors bors merged commit 10e85ef into rust-lang:master Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

format_push_string could use some more information
6 participants