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

Add check for assert_eq macros to unit_cmp lint #4613

Merged
merged 6 commits into from
Oct 4, 2019

Conversation

Lythenas
Copy link
Contributor

@Lythenas Lythenas commented Oct 2, 2019

changelog: Add check for unit comparisons through assert_eq!, debug_assert_eq!, assert_ne! and debug_assert_ne! macros to unit_cmp lint.

fixes #4481

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Thanks, only a NIT.

clippy_lints/src/types.rs Outdated Show resolved Hide resolved
@Lythenas
Copy link
Contributor Author

Lythenas commented Oct 3, 2019

Is it fine to use () as arguments in the tests? Or should I use blocks like with ==?

Should I also update the description of the lint to say it applies to asserts too?

@flip1995
Copy link
Member

flip1995 commented Oct 3, 2019

An update of the description of the lint would be good. And adding more tests is never wrong :)

@Lythenas
Copy link
Contributor Author

Lythenas commented Oct 3, 2019

Ok I will make changes to the description and the test. Also I just realized simply applying your suggestion through the github interface wasn't enough because I also need to update the .stderr 🤦‍♂️

@Lythenas
Copy link
Contributor Author

Lythenas commented Oct 3, 2019

Is there a way to preview the updated lint documentation?

@flip1995
Copy link
Member

flip1995 commented Oct 3, 2019

Is there a way to preview the updated lint documentation?

Kind of, but this isn't really necessary. But since you asked:

  1. You can export the lint documentation with the ./util/export.py script.
  2. After that you can open ./util/gh-pages/index.html with firefox or another browser
  3. (If you're using firefox >= 68.0 you have to set privacy.file_unique_origin in about:config to false before the page can be displayed. See: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSRequestNotHttp; other browsers may also have the same "problem", but I didn't look into how to fix it in other browsers 😉)

@Lythenas
Copy link
Contributor Author

Lythenas commented Oct 3, 2019

@flip1995 I think I'm done. Can you check if my changes to the doc is ok?

@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 4, 2019
@flip1995
Copy link
Member

flip1995 commented Oct 4, 2019

Looks great, thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2019

📌 Commit 5a0a2b3 has been approved by flip1995

bors added a commit that referenced this pull request Oct 4, 2019
Add check for assert_eq macros to unit_cmp lint

changelog: Add check for unit comparisons through `assert_eq!`, `debug_assert_eq!`, `assert_ne!` and `debug_assert_ne!` macros to unit_cmp lint.

fixes #4481
@bors
Copy link
Contributor

bors commented Oct 4, 2019

⌛ Testing commit 5a0a2b3 with merge 54bf4ff...

@bors
Copy link
Contributor

bors commented Oct 4, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 54bf4ff to master...

@bors bors merged commit 5a0a2b3 into rust-lang:master Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint: comparing two expressions of unit type
4 participants