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(sync): add lints to sync #1847

Merged
merged 1 commit into from
Nov 7, 2024
Merged

chore(sync): add lints to sync #1847

merged 1 commit into from
Nov 7, 2024

Conversation

giladchase
Copy link
Contributor

Lior banned as repo-wide, unless absolutely necessary.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.63%. Comparing base (e3165c4) to head (295c5c1).
Report is 240 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1847       +/-   ##
===========================================
- Coverage   40.10%   24.63%   -15.48%     
===========================================
  Files          26       79       +53     
  Lines        1895     8924     +7029     
  Branches     1895     8924     +7029     
===========================================
+ Hits          760     2198     +1438     
- Misses       1100     6414     +5314     
- Partials       35      312      +277     

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

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)


crates/papyrus_sync/src/lib.rs line 0 at r1 (raw file):
can you move the #[allow]s to above the problematic lines, instead of annotating blocks of code?

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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @elintul)


crates/papyrus_sync/src/lib.rs line at r1 (raw file):

Previously, dorimedini-starkware wrote…

can you move the #[allow]s to above the problematic lines, instead of annotating blocks of code?

I wish 🥲
The problematic lines here are gauge!(...), and #[allow(clippy doesn't work on macros, only on enclosing items.
So in all the places that included this macro I added the ignore at the closest enclosing non-macro parent.

In parallel I'm figuring out how I can test out the output of these metrics and see if switching from gauge! to counter! is equivalent output-wise (the latter take integer values and the former takes floats, even though our metrics are integers)

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware 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 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @elintul)

@giladchase giladchase force-pushed the gilad/storage-lints-1 branch 2 times, most recently from b230c2d to a55bfb3 Compare November 7, 2024 14:32
Lior banned `as` repo-wide, unless absolutely necessary.
@giladchase giladchase changed the base branch from gilad/storage-lints-1 to main November 7, 2024 14:32
@giladchase giladchase merged commit 5fe6be8 into main Nov 7, 2024
18 of 30 checks passed
@giladchase giladchase deleted the gilad/sync-lints branch November 7, 2024 17:31
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 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