-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add cargo dev rename_lint
#8625
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
Conversation
r? @xFrednet (rust-highfive has picked a reviewer for you, use r? to override) |
f3b6f61
to
b282bd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall, some minor things, but everything should be simple to fix. I also want to test it locally, but it looks very good so far. Thank you!
86f6c71
to
5bb7c1a
Compare
It should actually work now. Managed to flip the assert conditions without noticing at some point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've run a few tests locally and the result looks good. Some small comments and then it should be ready to be merged 🙃
0c6e559
to
4f28e71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version looks good to me. I've added one question, and I want to test it one more time before merging it. 🙃
Path::new(&format!("tests/ui/{}.rs", old_name)), | ||
Path::new(&format!("tests/ui/{}.rs", new_name)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an advantage in checking each path individually instead of iterating over every file and then only checking the file name regardless of the path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the easiest way to make sure the new name doesn't already exist before we start making changes based on the rename being successful. e.g. tests/ui/foo.fixed
shouldn't be renamed to tests/ui/bar.fixed
if tests/ui/bar.rs
already exists.
A similar issue happens with module names
I've tested this locally and got a weird error when running: RUST_BACKTRACE=1 cargo dev rename_lint debug_assert_with_mut_call muhhhhh It says that it tries to create a file called Running `target/debug/clippy_dev rename_lint debug_assert_with_mut_call muhhhhh`
thread 'main' panicked at 'failed to create file `clippy_lints/src/mutable_debug_assertion/muhhhhh.rs`: No such file or directory (os error 2)', src/update_lints.rs:765:5
stack backtrace:
0: rust_begin_unwind
at /rustc/9f4dc0b4db892271cd0dada6e072775b5b5d6b1e/library/std/src/panicking.rs:584:5
1: core::panicking::panic_fmt
at /rustc/9f4dc0b4db892271cd0dada6e072775b5b5d6b1e/library/core/src/panicking.rs:143:14
2: clippy_dev::update_lints::panic_file
at ./clippy_dev/src/update_lints.rs:765:5
3: clippy_dev::update_lints::try_rename_file
at ./clippy_dev/src/update_lints.rs:748:19
4: clippy_dev::update_lints::rename
at ./clippy_dev/src/update_lints.rs:277:16
5: clippy_dev::main
at ./clippy_dev/src/main.rs:66:13
6: core::ops::function::FnOnce::call_once
at /rustc/9f4dc0b4db892271cd0dada6e072775b5b5d6b1e/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. Something similar happens with
Could you maybe have a look at that? Afterwards, I'm happy to merge this PR 🙃 |
2ab3a9b
to
80d00be
Compare
I found one last bug, and then I believe it's ready. The following command replaces the content of cargo dev rename_lint expl_impl_clone_on_copy --uplift Other than that, everything looks good to me 🙃 |
Looks good to me, thank you for the new addition. @bors r+ |
📌 Commit b3de32b has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes #7799
changelog: None