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

New lint: result-unit-err #6157

Merged
merged 2 commits into from
Oct 11, 2020
Merged

New lint: result-unit-err #6157

merged 2 commits into from
Oct 11, 2020

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Oct 11, 2020

This fixes #1721.

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

changelog: new lint: result-unit-err

@rust-highfive
Copy link

r? @ebroto

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 11, 2020
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some thoughts.

I'm a bit hesitant about making it warn-by-default, beause I'm sure we are going to hear opinions on this :) but I would say that since it's warning only on public functions, it's more than desirable to not use this pattern there.

clippy_lints/src/functions.rs Outdated Show resolved Hide resolved
clippy_lints/src/functions.rs Outdated Show resolved Hide resolved
tests/ui/result_unit_error.rs Show resolved Hide resolved
clippy_lints/src/functions.rs Outdated Show resolved Hide resolved
clippy_lints/src/functions.rs Outdated Show resolved Hide resolved
clippy_lints/src/functions.rs Outdated Show resolved Hide resolved
@ebroto ebroto 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 Oct 11, 2020
@llogiq
Copy link
Contributor Author

llogiq commented Oct 11, 2020

Yeah, I was thinking about making it allow by default, but then it's only for the public interface, and missing the Error trait here is usually bad style. Non-newbies will find the lint to be easy to allow anyway. Perhaps I should add a check for no_std code where a Result<_, ()> is somewhat more likely to be acceptable?

Otherwise r?

Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I should add a check for no_std code where a Result<_, ()> is somewhat more likely to be acceptable

I think that's a good idea, but given that we lint only on () I think we can assume no_std users can use another type.

I did not see a nit before (error messages shouldn't be capitalized), I've changed it myself. When CI passes I'll merge this, thank you!

@ebroto
Copy link
Member

ebroto commented Oct 11, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 11, 2020

📌 Commit 74ae116 has been approved by ebroto

@bors
Copy link
Collaborator

bors commented Oct 11, 2020

⌛ Testing commit 74ae116 with merge 92783e3...

@bors
Copy link
Collaborator

bors commented Oct 11, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 92783e3 to master...

@bors bors merged commit 92783e3 into master Oct 11, 2020
@ebroto ebroto deleted the result-unit-err branch October 11, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint usage of Result<T, ()>
4 participants