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

Suggest is_some_and for map(<f>).unwrap_or(false) #10102

Closed
wants to merge 7 commits into from

Conversation

TheCodingWombat
Copy link

This is my first PR and contribution to Clippy.

changelog: map_unwrap_or: Added suggesting is_some_and when pattern Option.map(<f>).unwrap_or(false) occurs.

This is a first step in fixing all suggestions in issue #9125.

@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2022

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

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 19, 2022
@TheCodingWombat
Copy link
Author

I had help from @xFrednet in getting started with conributing to Clippy

@xFrednet
Copy link
Member

xFrednet commented Dec 19, 2022

Hey, awesome to see a PR for this!

I've just noticed that this suggestion requires the is_some_and feature. This is totally fine, but the documentation should clearly state that and the suggestion should only be made if the feature is enabled.

Usually, we use cx.tcx.sess.features_untracked().<featur-name> to check if a feature is enabled, but I can't find there rustc_feature::Features. I've ask on Zulip so see if the others know :)


It looks like, the CI is currently failing, as the doc comment also requires the feature. It should work if you add it in the markdown code block. It would be good, if you could also add a comment saying that this feature is required for the is_some_and() suggestion :).

I'll wait for an answer on Zulip, until I do a full review. You're welcome to reach out if you need any help :)


r? @xFrednet

@rustbot rustbot assigned xFrednet and unassigned dswij Dec 19, 2022
@Niki4tap
Copy link
Contributor

Coming back from zulip, the code you are looking for to check for a library feature is:

cx
    .tcx
    .sess
    .features_untracked()
    .declared_lib_features
    .iter()
    .any(|(name, _span)| name.as_str() == "is_some_and")

@TheCodingWombat
Copy link
Author

@xFrednet Check! Sounds useful to learn how to do that. Will do soon (tm).

@xFrednet
Copy link
Member

I'll mark this as waiting on author. GitHub will notify me when there are any updates. Enjoy the festive season!

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 28, 2022
@xFrednet
Copy link
Member

Hey @TheCodingWombat, this is a small ping from triage. Do you think you'll have time to continue this PR? 🙃

@bors
Copy link
Contributor

bors commented Feb 16, 2023

☔ The latest upstream changes (presumably #10163) made this pull request unmergeable. Please resolve the merge conflicts.

@xFrednet
Copy link
Member

Hey, I will closes this PR due to inactivity. This was already a very good start and I will link it in the issue for further development. If you have the time, @TheCodingWombat you're more than welcome to pick the issue up again. Thank you for the work you did in this PR :)

@xFrednet xFrednet closed this Mar 21, 2023
@xFrednet xFrednet added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 21, 2023
@jonboh
Copy link
Contributor

jonboh commented Oct 10, 2023

@rustbot label -S-inactive-closed

@rustbot rustbot removed the S-inactive-closed Status: Closed due to inactivity label Oct 10, 2023
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.

7 participants