-
Notifications
You must be signed in to change notification settings - Fork 1.7k
new unnecessary_map_on_constructor lint #11413
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
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
70ea595
to
9cf4829
Compare
☔ The latest upstream changes (presumably #10692) made this pull request unmergeable. Please resolve the merge conflicts. |
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! Looks good, got a couple macro cases but mostly style nits
Thanks for the comments :), I think I've addressed all issues. From the Zulip thread I get that the new syntax for the comments in the UI tests is still in the works, I've removed the comments, but if there's already some agreed upon syntax that I can add to be aligned with future practices let me know. |
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! just a couple last things I spotted and it looks good. If you could squash the commits too that would be great then it's ready for merging
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged. You can start a rebase with the following commands:
The following commits are merge commits: |
Great, thanks! @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
1 similar comment
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changelog: [
unnecessary_map_on_constructor
]: adds lint for cases in which map is not necessary.Some(4).map(myfunction)
=>Some(myfunction(4))
Closes #6472
Note that the case mentioned in the issue
Some(..).and_then(|..| Some(..))
is fixed by a chain of lint changes. This PR completes the last part of that chain.By
bind_instead_of_map
lint:Some(4).and_then(|x| Some(foo(4)))
=>Some(4).map(|x| foo)
By
redundant_closure
lint:Some(4).map(|x| foo)
=>Some(4).map(fun)
Finally by this PR
unnecessary_map_on_constructor
:Some(4).map(fun)
=>Some(fun(4))
I'm not sure this is the desired behavior for clippy and if it should be addressed in another issue/PR. I'd be up to give it a try if that's the case.