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.

Sign up for GitHub

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

Jump to bottom

[move][clippy] Apply clippy CI check to all targets #17165

Merged
merged 5 commits into from
Apr 16, 2024
Merged

[move][clippy] Apply clippy CI check to all targets #17165

merged 5 commits into from
Apr 16, 2024

Conversation

tnowacki
Copy link
Contributor

Description

  • cargo move-clippy did not include tests

Test plan

  • CI

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

@tnowacki tnowacki requested a review from a team April 15, 2024 23:16
@vercel Vercel
Copy link

vercel bot commented Apr 15, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2024 11:54pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Apr 15, 2024 11:54pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Apr 15, 2024 11:54pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Apr 15, 2024 11:54pm

@tnowacki tnowacki marked this pull request as ready for review April 15, 2024 23:16
tzakian
tzakian approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@tzakian tzakian left a comment

Choose a reason for hiding this comment

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

LGTM, couple small nits. What cargo fmt did in one place made me sad :'( but nothing to do about it.

external-crates/move/crates/bytecode-verifier-tests/src/unit_tests/bounds_tests.rs Outdated
@@ -222,16 +222,14 @@ fn invalid_signature_for_vector_operation() {

let skeleton = basic_test_module();
let sig_index = SignatureIndex(skeleton.signatures.len() as u16);
for bytecode in vec![
VecPack(sig_index, 0),
for bytecode in [VecPack(sig_index, 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

ick is this what cargo fmt does for this? 🤢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it doesn't. Our rustfmt is really fucked for external-crates

external-crates/move/crates/move-analyzer/src/symbols.rs Outdated
@@ -3807,11 +3807,10 @@ fn assert_use_def_with_doc_string(
"for use in column {use_col} of line {use_line} in file {use_file}"
);
let info = def_info.get(&use_def.def_loc).unwrap();
let info_str = format!("{}", info);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
let info_str = format!("{}", info);
let info_str = info.to_string();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it implements to_string? Or does anything that has Display has to_string?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct -- anything that is Display has to_string -- to_string calls Display underneath it https://doc.rust-lang.org/std/string/trait.ToString.html

external-crates/move/crates/move-stackless-bytecode/src/borrow_analysis.rs
self.borrows_from
.get(node)
.map(|s| s.iter().map(|(n, e)| (n, e)).collect_vec())
Copy link
Contributor

Choose a reason for hiding this comment

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

@tnowacki tnowacki merged commit 5aea922 into MystenLabs:main Apr 16, 2024
44 checks passed
@tnowacki tnowacki deleted the clippy-fix branch April 16, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzakian tzakian tzakian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tnowacki @tzakian