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

chore(node): add workspace lints #1830

Merged
merged 2 commits into from
Nov 7, 2024
Merged

chore(node): add workspace lints #1830

merged 2 commits into from
Nov 7, 2024

Conversation

giladchase
Copy link
Contributor

Lior banned as repo-wide, unless absolutely necessary.

Nearly all as uses can be replaced with [Try]From which doesn't have implicit coercions like as (we encountered several bugs due to these coercions).

Motivation: we are standardizing lints across the repo and CI, instead of each crate having separate sets of lints.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @eitanm-starkware and @elintul)


crates/papyrus_node/examples/get_transaction_hash.rs line 82 at r1 (raw file):

        .get_one::<String>("concurrent_requests")
        .expect("Failed parsing concurrent_requests")
        .parse::<usize>()

Turbofish not needed here, but kept it to be consistent with the surrounding code, and changing all surrounding code is OOS

Code quote:

        .parse::<usize>()

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @eitanm-starkware)

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 7.17%. Comparing base (e3165c4) to head (d06c9d6).
Report is 226 commits behind head on main.

Files with missing lines Patch % Lines
crates/papyrus_network/src/network_manager/mod.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #1830       +/-   ##
==========================================
- Coverage   40.10%   7.17%   -32.94%     
==========================================
  Files          26     213      +187     
  Lines        1895   30066    +28171     
  Branches     1895   30066    +28171     
==========================================
+ Hits          760    2156     +1396     
- Misses       1100   27709    +26609     
- Partials       35     201      +166     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @eitanm-starkware)

@giladchase giladchase force-pushed the gilad/node-lints branch 2 times, most recently from a9ec62e to d48b96b Compare November 5, 2024 12:54
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @eitanm-starkware)

Lior banned `as` repo-wide, unless absolutely necessary.

Nearly all as uses can be replaced with [Try]From which doesn't have
implicit coercions like as (we encountered several bugs due to these coercions).

Motivation: we are standardizing lints across the repo and CI,
instead of each crate having separate sets of lints.

Note: i left the `as f64` since `gauge!` expects f64, even though the
metrics themselves are integers. This is probably fixable by using
`counter!` instead of `gauge!`, but i haven't found anyone who knows how
to test these metrics so i'm leaving it for now.
I left them at the statement level intentionaly, instead of the module level,
so that it won't be forgotten.
Lior banned `as` repo-wide, unless absolutely necessary.

Nearly all as uses can be replaced with [Try]From which doesn't
have implicit coercions like as (we encountered several bugs due to these coercions).

Motivation: we are standardizing lints across the repo and CI,
instead of each crate having separate sets of lints.

Note: the change in pointers.rs is due to cargo doc being triggered due
to changes in the crate, and cargo doc thought it was a busted HTML tag
:sweat_smile:
Base automatically changed from gilad/network-lints to main November 7, 2024 08:04
@giladchase giladchase merged commit 880df26 into main Nov 7, 2024
30 checks passed
@giladchase giladchase deleted the gilad/node-lints branch November 7, 2024 08:27
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants