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/clarity fix clippy #3946

Merged
merged 11 commits into from
Sep 22, 2023
Merged

Chore/clarity fix clippy #3946

merged 11 commits into from
Sep 22, 2023

Conversation

jferrant
Copy link
Collaborator

Closes #3935

I had time to kill while waiting for my node to spin up...

To run clippy on clarity ONLY:

cargo clippy -p clarity --no-deps --tests

@jferrant jferrant added the chore Necessary but less impactful tasks such as cleanup or reorg label Sep 19, 2023
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #3946 (3af3051) into develop (904c7ab) will increase coverage by 0.00%.
Report is 3 commits behind head on develop.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop    #3946     +/-   ##
==========================================
  Coverage     0.16%    0.16%             
==========================================
  Files          339      339             
  Lines       290230   287388   -2842     
==========================================
  Hits           469      469             
+ Misses      289761   286919   -2842     
Files Changed Coverage Δ
clarity/src/libclarity.rs 0.00% <ø> (ø)
clarity/src/vm/analysis/analysis_db.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/arithmetic_checker/mod.rs 0.00% <0.00%> (ø)
.../src/vm/analysis/contract_interface_builder/mod.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/errors.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/mod.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/read_only_checker/mod.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/trait_checker/mod.rs 0.00% <0.00%> (ø)
clarity/src/vm/analysis/type_checker/contexts.rs 0.00% <0.00%> (ø)
...ity/src/vm/analysis/type_checker/v2_05/contexts.rs 0.00% <0.00%> (ø)
... and 65 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

This LGTM, but I just want to make sure @obycode and @kantai are aware of this. They may have opinions on when the appropriate time to merge it will be.

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for taking this on. I'll happily rebase on top of this and clippy my changes going forward.

Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@obycode
Copy link
Contributor

obycode commented Sep 22, 2023

@jferrant when you need to merge in upstream changes, it's better to use a merge commit, rather than rebasing and force-pushing, since the latter makes it a lot harder for reviewers to see what changed.

@diwakergupta diwakergupta merged commit fdae394 into develop Sep 22, 2023
2 checks passed
@jferrant
Copy link
Collaborator Author

@jferrant when you need to merge in upstream changes, it's better to use a merge commit, rather than rebasing and force-pushing, since the latter makes it a lot harder for reviewers to see what changed.

I am much more of a fan of linear history and avoid merge commits as best as possible, but understand it can make it difficult to see which commit changes have been made since last review without manually inspecting.

@obycode obycode deleted the chore/clarity-fix-clippy branch September 22, 2023 18:38
@obycode
Copy link
Contributor

obycode commented Sep 22, 2023

I am much more of a fan of linear history and avoid merge commits as best as possible, but understand it can make it difficult to see which commit changes have been made since last review without manually inspecting.

I agree 💯 about linear history, and that's always been my preference too, but I'm not sure how to do that while making our other processes work smoothly. It's no good if GitHub can't show reviewers "changes since my last review". I don't think the tooling is there to make it work right.

So I gave up on the linear history and have embraced the merge commits 😰.

@jferrant
Copy link
Collaborator Author

jferrant commented Sep 22, 2023

I am much more of a fan of linear history and avoid merge commits as best as possible, but understand it can make it difficult to see which commit changes have been made since last review without manually inspecting.

I agree 💯 about linear history, and that's always been my preference too, but I'm not sure how to do that while making our other processes work smoothly. It's no good if GitHub can't show reviewers "changes since my last review". I don't think the tooling is there to make it work right.

So I gave up on the linear history and have embraced the merge commits 😰.

Oh no! Your soul was sufficiently crushed! Something tells me this may happen to me too XD I do remember the nightmare of a time I had trying to rebase next onto a sep branch I had in order to extract out hte sbtc changes...But I did manage to get it done! and it was so easy to find the buggy code that it ....ALMOST....made the 2 days I spent rebasing worth it XD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Necessary but less impactful tasks such as cleanup or reorg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants