4

Add `unnecessary_literal_unwrap` lint by pksunkara · Pull Request #10358 · rust-...

 1 year ago
source link: https://github.com/rust-lang/rust-clippy/pull/10358
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Add unnecessary_literal_unwrap lint #10358

Conversation

Contributor

Add lint for more unnecessary unwraps and suggest fixes for them.

Fixes #10352

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

r? @llogiq

changelog: New lint [unnecessary_literal_unwrap]
#10358

Collaborator

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

Please see the contribution instructions for more information.

rustbot

added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label

Feb 16, 2023

pksunkara

changed the title Add test code

Add unnecessary_literal_unwrap lint

Feb 16, 2023

Contributor

The unwrap_or_else_default UI test is still failing.

Contributor

@rustbot author

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

Feb 16, 2023

Contributor

The unwrap_or_else_default UI test is still failing.

As well as many others. I count differences in 21 output files when testing locally.

Contributor

Author

Only the documentation is left. But before I do that, I would like to know 2 things:

  1. How do I make this lint warn by default?
  2. What should I put in place of #[clippy::version = "1.45.0"] in declare_lint! usage?

Contributor

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

Contributor

@pksunkara do you intend to revisit this at some point?

Contributor

Author

Yup, I can take a look on Monday.

Contributor

Author

I would appreciate if I can get some answers for the questions I asked earlier, #10358 (comment)
llogiq reacted with thumbs up emoji

Contributor

  1. How do I make this lint warn by default?
  2. What should I put in place of #[clippy::version = "1.45.0"] in declare_lint! usage?
  1. Do you mean in a test? That would be #[warn(clippy::unnecessary_literal_unwrap)]. Otherwise the default lint level is given by the lint group. As this lint is in complexity, that should IIRC warn by default.
  2. #[clippy::version = "1.69.0"].
pksunkara reacted with thumbs up emoji

Contributor

Author

I have addressed the review comments.

@rustbot reviewer

Collaborator

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

rustbot

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

labels

Jun 7, 2023

Contributor

@pksunkara The comment to remove the L-waiting-on-author label is rustbot ready

Contributor

There are still merge conflicts, and I don't have time to review right now, sorry. Perhaps I'll find a few minutes tomorrow.
pksunkara reacted with thumbs up emoji

Contributor

Author

There are still merge conflicts

They are very minor ones because of this PR touching a lot of tests and I will fix them once the code is reviewed.

Contributor

@llogiq llogiq

left a comment

Looks good, I only have a small nit regarding docs.

Also I'm not completely sure about the name: So far we have 23 needless-* and 18 unnecessary-* lints. I like the former more for both brevity and being easier to write.

clippy_lints/src/methods/mod.rs

Outdated Show resolved

Contributor

Author

Addressed the conflicts and review comment.

Contributor

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

Contributor

So you intend to keep the name longer. Ah, well, you've done enough rebases as is. Let's roll with it. Thanks for seeing this PR through.

@bors r+
pksunkara reacted with thumbs up emoji

Contributor

pushpin Commit bfd5aba has been approved by llogiq

It is now in the queue for this repository.

Contributor

hourglass Testing commit bfd5aba with merge 7c2bf28...

bors

merged commit 7c2bf28 into

rust-lang:master

Jun 12, 2023

8 checks passed

pksunkara

deleted the unnecessary-unwrap branch

June 12, 2023 18:24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

samueltardieu

samueltardieu left review comments

llogiq

llogiq approved these changes
Assignees

llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unnecessary direct unwraps on Option and Result are not warned/fixed

7 participants

report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK