-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Move format_push_string to restriction #9161
Conversation
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. |
Hey, author of #9077. Thank you! If we want to move the lint, I'd like to note that using 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 |
I totally forgot that restriction even existed and I agree that pedantic wasn't the best fit. I've moved it to restriction now :) |
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! 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.
Just going to add a couple points here.
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 |
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) |
@flip1995 Added the known problems section :) |
@bors r+ Thanks! |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
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).