Added new lint "path_join_correction" #11208

ofeeg · 2023-07-21T18:19:48Z

r? xFrednet
changelog: Remade #10663 to point to a fresh branch as per request of @blyxyas

Finally got a break from my job to get around to this. Thank you guys for waiting.

rustbot · 2023-07-21T18:19:52Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

xFrednet

Hey @ofeeg, welcome back! This version is already a good start. I left several comments, once they are addressed, we should have a working lint. Please don't hesitate to reach out if you have any questions!

@rustbot author

CHANGELOG.md

clippy_lints/src/methods/join_absolute_path.rs

xFrednet · 2023-07-24T10:10:51Z

clippy_lints/src/methods/join_absolute_path.rs

+      span_lint_and_sugg(
+      cx,
+      JOIN_ABSOLUTE_PATH,
+      span.with_hi(expr.span.hi()),


Here you can just use join_arg.span :)

clippy_lints/src/methods/join_absolute_path.rs

clippy_lints/src/methods/mod.rs

xFrednet · 2023-07-24T10:28:37Z

clippy_lints/src/methods/mod.rs

+    /// let path = std::path::Path::new("/bin");
+    /// let res = path.join("/sh");
+    /// assert_eq!(res, std::path::PathBuf::from("/sh"));


In these examples, it would be nicer to use # use std::path::Path and then remove the leading std::path.

Suggested change

/// let path = std::path::Path::new("/bin");

/// let res = path.join("/sh");

/// assert_eq!(res, std::path::PathBuf::from("/sh"));

/// # use std::path::{Path, PathBuf}

/// let path = Path::new("/bin");

/// let joined_path = path.join("/sh");

/// assert_eq!(joined_path, PathBuf::from("/sh"));

Same in the example below

xFrednet · 2023-07-24T10:35:33Z

tests/ui/join_absolute_path.rs

@@ -0,0 +1,26 @@
+//@run-rustfix


rustfix only works with valid suggestions, for now, the best action is to rust remove it :)

Suggested change

//@run-rustfix

clippy_lints/src/methods/mod.rs

clippy_lints/src/methods/join_absolute_path.rs

blyxyas · 2023-07-24T23:24:19Z

"Pretty good first PR!"

Proceeds to completely and utterly destroy your PR

blyxyas · 2023-07-25T00:35:21Z

I'll make a PR against your fork in a few hours

I finally got around to making this PR and it seems like it's solved (cargo uitest has no errors). There's still the cargo dev fmt issue but that's minor.

Is it solved or do I save your fork just in case. Also, @xFrednet I think I'll let you have this one, as I have a lot of reviews currently. Hope it's not a problem :)

xFrednet · 2023-07-25T08:22:25Z

Is it solved or do I save your fork just in case. Also, @xFrednet I think I'll let you have this one, as I have a lot of reviews currently. Hope it's not a problem :)

I'm happy to do the full review :D

rustbot · 2023-07-27T15:13:15Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

ofeeg · 2023-07-27T15:19:56Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:
$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease
The following commits are merge commits:
* [1f26d8c](https://github.com/rust-lang/rust-clippy/commit/1f26d8cbd387f0bd459dfdaff44154909b8850df)

* [1d77675](https://github.com/rust-lang/rust-clippy/commit/1d7767588cbaa967c8f40128866470cb7ac78256)

I don't know what this is about. It may have to do with me accidentally committing to master instead of the new branch by complete accident. I gotta go to my day job now, though.

xFrednet · 2023-08-01T11:53:39Z

Clippy has a policy that branches should be updated using rebases instead of merges. I think in this case, it would be best to squash all commits into one. That would remove the merge commits, without any hassle.

Also, a small note, your most recent commits use a different name and don't link to your GH. This is fine for Clippy, but I wanted to mention it in case this was unintenional :)

ofeeg · 2023-08-03T14:53:29Z

Clippy has a policy that branches should be updated using rebases instead of merges. I think in this case, it would be best to squash all commits into one. That would remove the merge commits, without any hassle.

Also, a small note, your most recent commits use a different name and don't link to your GH. This is fine for Clippy, but I wanted to mention it in case this was unintenional :)

Thanks for your guidance! Hopefully this next commit is agreeable! Sorry for taking so long; I've been going through a lot of personal changes, and they're mostly related to getting out of intense physical labor for at least a little while. lol

xFrednet

This is a good step, the lint now detects the test cases correctly. I had some other suggests in the previous review, which were marked as Outdated since the file was renamed with the lint rename. Could you take a look at the resolved comments? They're related to the diagnostic and documentation style. Once they're addressed, we should be able to move forward :)

Sorry for taking so long; I've been going through a lot of personal changes, and they're mostly related to getting out of intense physical labor for at least a little while. lol

No problem at all. I hope this is a good change for you! :)

clippy_lints/src/methods/join_absolute_paths.rs~

Centri3

Just a couple nits, the actual code looks pretty good

clippy_lints/src/methods/mod.rs

clippy_lints/src/methods/join_absolute_paths.rs

clippy_lints/src/methods/mod.rs

tests/ui/join_absolute_paths.rs

clippy_lints/src/methods/mod.rs

xFrednet

This version looks good to me! Thank you for resolving all our comments. Also, thank you for your review @Centri3. ❤️

@ofeeg The commit history needs to be cleaned up a bit, before we can merge this. There should be no merge commits and the messages should all be descriptive. I would suggest squashing all commits of this PR into a single commit. This will remove the merge commits easily. Removing them with a rebase is possible, but can be a bit tricky.

And then we can merge it :)

ofeeg · 2023-08-14T16:38:42Z

This version looks good to me! Thank you for resolving all our comments. Also, thank you for your review @Centri3. heart

@ofeeg The commit history needs to be cleaned up a bit, before we can merge this. There should be no merge commits and the messages should all be descriptive. I would suggest squashing all commits of this PR into a single commit. This will remove the merge commits easily. Removing them with a rebase is possible, but can be a bit tricky.

And then we can merge it :)

I'm still learning how to rebase effectively. I think I did it correctly but even with no changes from the passing commit that I backed up it seems to be failing the test here on github. I'm pretty confused as to how to handle this due to all the squashing I had to do.

xFrednet · 2023-08-18T15:41:47Z

I've tried to look how this can be easily fixed, but I couln't find a good solution. Rebases can be really evil, especially when you only get started using them 🙈. In this case, I would suggest the following:

Create a backup branch
Reset this branch to have no changes at all
Copy the files you touched onto this branch
Add a commit for them

This is far from ideal, but the best way I can find right now. I have a backup of an older version of this PR under https://github.com/xFrednet/rust-clippy/tree/backup/pr/path_join_correct that branch might be a good start, if you want to see which files you touched.

Sorry that the rebase has messed up the history so much. The next time, I'll try to write more detailed instruction 🙈

ofeeg · 2023-08-27T15:03:12Z

I think I fixed everything, but I did a force push which seems to be an awful idea. I ran cargo test on my own machine and it worked so should I just create a new PR again, or are you able to reopen the PR upon checking the recent commit? @xFrednet

xFrednet · 2023-09-02T21:44:39Z

Hey @ofeeg, something is still of, but I'm not quite sure what. I created #11453 which should include all theses changes :)

New lint `clippy::join_absolute_paths` Hey `@ofeeg,` this PR is a copy of all the changes you did in 47171d3 from PR #11208. I wasn't sure how to fix the git history. Hope you're okay with me figuring this out in this separate PR --- changelog: New lint [`join_absolute_paths`] [#11453](#11453) Closes: #10655 r? `@Centri3` Since you also gave feedback on the other PR. I hope that I copied everything correctly, but a third pair of eyes would be appreciated :D

rustbot assigned xFrednet Jul 21, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 21, 2023

xFrednet reviewed Jul 24, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 24, 2023

xFrednet reviewed Aug 6, 2023

View reviewed changes

clippy_lints/src/methods/join_absolute_paths.rs~ Outdated Show resolved Hide resolved

Centri3 reviewed Aug 6, 2023

View reviewed changes

xFrednet reviewed Aug 12, 2023

View reviewed changes

ofeeg force-pushed the path_join_correct branch from 47171d3 to 0c71347 Compare August 13, 2023 15:20

ofeeg closed this Aug 14, 2023

ofeeg reopened this Aug 14, 2023

ofeeg force-pushed the path_join_correct branch from b871e39 to b868cbb Compare August 15, 2023 15:12

Commit with just personally changed files.

f43bc18

ofeeg closed this Aug 27, 2023

ofeeg force-pushed the path_join_correct branch from e103e8f to f43bc18 Compare August 27, 2023 14:27

xFrednet mentioned this pull request Sep 2, 2023

New lint clippy::join_absolute_paths #11453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new lint "path_join_correction" #11208

Added new lint "path_join_correction" #11208

ofeeg commented Jul 21, 2023

rustbot commented Jul 21, 2023

xFrednet left a comment

xFrednet Jul 24, 2023

xFrednet Jul 24, 2023

xFrednet Jul 24, 2023

blyxyas commented Jul 24, 2023

blyxyas commented Jul 25, 2023

xFrednet commented Jul 25, 2023

rustbot commented Jul 27, 2023

ofeeg commented Jul 27, 2023

xFrednet commented Aug 1, 2023

ofeeg commented Aug 3, 2023

xFrednet left a comment •

edited

Loading

Centri3 left a comment

xFrednet left a comment

ofeeg commented Aug 14, 2023

xFrednet commented Aug 18, 2023

ofeeg commented Aug 27, 2023

xFrednet commented Sep 2, 2023

-    /// let path = std::path::Path::new("/bin");
-    /// let res = path.join("/sh");
-    /// assert_eq!(res, std::path::PathBuf::from("/sh"));
+    /// # use std::path::{Path, PathBuf}
+    /// let path = Path::new("/bin");
+    /// let joined_path = path.join("/sh");
+    /// assert_eq!(joined_path, PathBuf::from("/sh"));

Added new lint "path_join_correction" #11208

Added new lint "path_join_correction" #11208

Conversation

ofeeg commented Jul 21, 2023

rustbot commented Jul 21, 2023

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Jul 24, 2023

Choose a reason for hiding this comment

xFrednet Jul 24, 2023

Choose a reason for hiding this comment

xFrednet Jul 24, 2023

Choose a reason for hiding this comment

blyxyas commented Jul 24, 2023

blyxyas commented Jul 25, 2023

xFrednet commented Jul 25, 2023

rustbot commented Jul 27, 2023

ofeeg commented Jul 27, 2023

xFrednet commented Aug 1, 2023

ofeeg commented Aug 3, 2023

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

Centri3 left a comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

ofeeg commented Aug 14, 2023

xFrednet commented Aug 18, 2023

ofeeg commented Aug 27, 2023

xFrednet commented Sep 2, 2023

xFrednet left a comment •

edited

Loading