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

Revisit Clippy whitelisting #442

Closed
bowenwang1996 opened this issue Jan 20, 2019 · 9 comments
Closed

Revisit Clippy whitelisting #442

bowenwang1996 opened this issue Jan 20, 2019 · 9 comments
Labels
B-L1 Bootcamp: (“Nit”) minor 1-5 code line change C-housekeeping Category: Refactoring, cleanups, code quality

Comments

@bowenwang1996
Copy link
Collaborator

bowenwang1996 commented Jan 20, 2019

We should start enabling [clippy](https://github.com/rust-lang/rust-clippy) gradually. To do that, we should first figure out which rules trigger warnings and document them. The purpose of this issue is to get a list of clippy rules that we do not follow now so that we can fix them in the future.

@bowenwang1996 bowenwang1996 added C-housekeeping Category: Refactoring, cleanups, code quality P-low Priority: low labels Jan 20, 2019
@bowenwang1996 bowenwang1996 added this to the MVB milestone Jan 20, 2019
@ilblackdragon ilblackdragon modified the milestones: AlphaNet, BetaNet Mar 28, 2019
@ilblackdragon ilblackdragon removed this from the TestNet 2.0 milestone Jul 2, 2019
@evgenykuzyakov evgenykuzyakov added Priority 9001 and removed P-low Priority: low labels Aug 29, 2019
@ilblackdragon ilblackdragon added this to the Post MainNet milestone Nov 22, 2019
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
birchmd added a commit to birchmd/nearcore that referenced this issue May 12, 2020
nearprotocol-bulldozer bot pushed a commit that referenced this issue May 14, 2020
fix: `clippy::perf`, `clippy::correctness` (#442)

The following command now returns without error:

```
cargo clippy --workspace -- -Aclippy::all -Dclippy::perf -Dclippy::correctness
```

The lints which have been corrected are:
clippy::redundant_clone
clippy::useless_vec
clippy::trivially_copy_pass_by_ref
clippy::expect_fun_call
clippy::inefficient_to_string
clippy::or_fun_call
clippy::map_entry
clippy::absurd_extreme_comparisons
clippy::clone_double_ref
clippy::single_char_pattern
clippy::large_enum_variant
@bowenwang1996 bowenwang1996 removed their assignment Jun 3, 2021
@bowenwang1996 bowenwang1996 added bootcamp and removed C-housekeeping Category: Refactoring, cleanups, code quality labels Jun 3, 2021
@bowenwang1996 bowenwang1996 removed this from the Post MainNet milestone Jun 3, 2021
@frol frol added C-housekeeping Category: Refactoring, cleanups, code quality B-L1 Bootcamp: (“Nit”) minor 1-5 code line change labels Jun 3, 2021
@frol
Copy link
Collaborator

frol commented Jun 4, 2021

I assigned L1 implying that initial items should not be huge and Clippy + Rust compiler should guide us through, but if a bigger refactoring is necessary, we should decouple those into separate dedicated issues.

@stale
Copy link

stale bot commented Sep 2, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Sep 2, 2021
@stale
Copy link

stale bot commented Dec 1, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Dec 1, 2021
@bowenwang1996
Copy link
Collaborator Author

@matklad @nagisa curious what your thoughts are on this topic

@matklad
Copy link
Contributor

matklad commented Dec 2, 2021

While I am not a huge fan of clippy, for a big project with many developers, like nearcore, I think enabling clippy would be a net benefit, so we should do that.

Though, in terms of "keeping the code base sane", I don't think that's the most impactful thing. I'd say we should fix #4490 and improve compile times (by auditing our deps in Cargo.lock and cutting the cruft out) first.

@nagisa
Copy link
Collaborator

nagisa commented Dec 2, 2021

i definitely seen bugs being caught by clippy in CI, so I'm in favour of adding some of the clippy lints to at least CI.

@pmnoxx
Copy link
Contributor

pmnoxx commented Jan 3, 2022

I'm a big favor of clippy. I detected many potential issues ahead of time.
It also has many useful suggestions on how to write a cleaner, more readable code.I think we should enable it by default.

@stale
Copy link

stale bot commented Apr 3, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Apr 3, 2022
@akhi3030 akhi3030 removed the S-stale label Jul 8, 2022
@akhi3030
Copy link
Collaborator

Closing in favour of #8145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-L1 Bootcamp: (“Nit”) minor 1-5 code line change C-housekeeping Category: Refactoring, cleanups, code quality
Projects
None yet
Development

No branches or pull requests

9 participants