Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Clippyfy #6341

Merged
10 commits merged into from
Nov 30, 2022
Merged

Clippyfy #6341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a57a196
Add clippy config and remove .cargo from gitignore
Nov 23, 2022
00c05ec
first fixes
Nov 23, 2022
c4d80f0
Clippyfied
Nov 24, 2022
5fc79c9
Merge branch 'master' into alex/clippyfy
Nov 24, 2022
81fd2a8
Add clippy CI job
Nov 24, 2022
d96fd47
comment out rusty-cachier
Nov 24, 2022
c513688
minor
Nov 24, 2022
e09df27
fix ci
alvicsam Nov 25, 2022
0b70674
remove DAG from check-dependent-project
alvicsam Nov 25, 2022
c27890a
add DAG to clippy
alvicsam Nov 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add clippy config and remove .cargo from gitignore
  • Loading branch information
alexgparity committed Nov 23, 2022
commit a57a196b61ce1af09fd5cddf1a1dff337600d8bc
32 changes: 32 additions & 0 deletions .cargo/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#
# An auto defined `clippy` feature was introduced,
# but it was found to clash with user defined features,
# so was renamed to `cargo-clippy`.
#
Comment on lines +1 to +5
Copy link
Contributor

@mrcnski mrcnski Nov 24, 2022

Choose a reason for hiding this comment

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

nit: Interesting history, but unrelated to our codebase, I think this is unneeded.

# If you want standard clippy run:
# RUSTFLAGS= cargo clippy
[target.'cfg(feature = "cargo-clippy")']
rustflags = [
"-Aclippy::all",
"-Dclippy::correctness",
"-Aclippy::if-same-then-else",
"-Aclippy::clone-double-ref",
"-Dclippy::complexity",
"-Aclippy::zero-prefixed-literal", # 00_1000_000
"-Aclippy::type_complexity", # raison d'etre
"-Aclippy::nonminimal-bool", # maybe
"-Aclippy::borrowed-box", # Reasonable to fix this one
"-Aclippy::too-many-arguments", # (Turning this on would lead to)
Copy link
Contributor

@mrcnski mrcnski Nov 24, 2022

Choose a reason for hiding this comment

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

Suggested change
"-Aclippy::too-many-arguments", # (Turning this on would lead to)
"-Aclippy::too-many-arguments", # (Turning this on would lead to too many errors)

"-Aclippy::unnecessary_cast", # Types may change
"-Aclippy::identity-op", # One case where we do 0 +
"-Aclippy::useless_conversion", # Types may change
Copy link
Contributor

@mrcnski mrcnski Nov 24, 2022

Choose a reason for hiding this comment

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

Looks like these lints were copied from Substrate. But it makes me sad that this is allow. We have a bunch of easy simplifications like e.g.

  --> xcm/src/v1/junction.rs:95:66
   |
95 |             Junction0::Plurality { id, part } => Ok(Self::Plurality { id: id.into(), part }),
   |                                                                           ^^^^^^^^^ help: consider removing `.into()`: `id`

This lint can be annoying during development as "Types may change", but in CI it should be more strict IMO. That would keep friction to a minimum.

Probably this should be a follow-up issue where people can chime in. Super low priority in the grand scheme of things, so let's leave the lints as-is for now.

Copy link
Author

@ghost ghost Nov 24, 2022

Choose a reason for hiding this comment

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

That also really depends on people's workflow. I only run clippy manually when I'm done. I.e. that lint wouldn't bother me at all.

Copy link
Contributor

@mrcnski mrcnski Nov 24, 2022

Choose a reason for hiding this comment

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

Yeah, I guess you can just have clippy in your pre-push hook instead of pre-commit.

Also I remembered you can #[allow(clippy::lint)] on certain lines. I would prefer that as opposed to white-listing a lint for the whole codebase for "One case where we do 0 +".

"-Aclippy::unit_arg", # styalistic.
"-Aclippy::option-map-unit-fn", # styalistic
"-Aclippy::bind_instead_of_map", # styalistic
Comment on lines +23 to +25
Copy link
Contributor

@mrcnski mrcnski Nov 24, 2022

Choose a reason for hiding this comment

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

Suggested change
"-Aclippy::unit_arg", # styalistic.
"-Aclippy::option-map-unit-fn", # styalistic
"-Aclippy::bind_instead_of_map", # styalistic
"-Aclippy::unit_arg", # stylistic.
"-Aclippy::option-map-unit-fn", # stylistic
"-Aclippy::bind_instead_of_map", # stylistic

"-Aclippy::erasing_op", # E.g. 0 * DOLLARS
"-Aclippy::eq_op", # In tests we test equality.
"-Aclippy::while_immutable_condition", # false positives
"-Aclippy::needless_option_as_deref", # false positives
"-Aclippy::derivable_impls", # false positives
"-Aclippy::stable_sort_primitive", # prefer stable sort
]
1 change: 0 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,4 @@ polkadot.*
!polkadot.service
!.rpm/*
.DS_Store
.cargo
.env