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

New lint [absolute_paths] #11003

Merged
merged 1 commit into from
Jul 21, 2023
Merged

New lint [absolute_paths] #11003

merged 1 commit into from
Jul 21, 2023

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jun 22, 2023

Closes #10568

Maybe we should make the max segments allowed a configuration option? I see quite a bit of 3-segment paths in clippy, and while I think only really <mod/type>::<item> or <item> should be (usually) used but anything above may be too widespread 😕

PS, despite this being "max segments allowed" it only lints if it's absolute, as is the point of the lint, e.g., std::io::ErrorKind::etc is linted but io::ErrorKind::NotFound::etc isn't

changelog: New lint [absolute_paths]

@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2023

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 22, 2023
@bors
Copy link
Contributor

bors commented Jun 27, 2023

☔ The latest upstream changes (presumably #10884) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo
Copy link
Member

Would you like to take a look at this one @blyxyas? (If you have a bunch on your plate already please say so)

@blyxyas
Copy link
Member

blyxyas commented Jun 28, 2023

Yeah, I'll take a look!

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

I think we should have an option to enable std uses with any levels. For example, in my projects I usually leave std functions with their absolute paths to show that they are from the standard library.

clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
tests/ui/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
@Centri3
Copy link
Member Author

Centri3 commented Jun 28, 2023

in my projects I usually leave std functions with their absolute paths to show that they are from the standard library.

ra actually colors such functions differently, not sure whether that works across all IDEs though

@blyxyas
Copy link
Member

blyxyas commented Jun 28, 2023

ra actually colors such functions differently

This is the first time I hear about this 🤯
But I still thinking having a config value to allow those paths would be neat, as probably a lot of people don't know that that happens.

@blyxyas
Copy link
Member

blyxyas commented Jun 28, 2023

image

Update: I'm not seeing those colors 😞
Edit: Not even on the pre-release version? What version are you on?

@Centri3
Copy link
Member Author

Centri3 commented Jun 28, 2023

Hmm weird. I'm using one dark pro with vscode (yes, I'm boring) and I see this:

image

What theme are you using?

I'm on a slightly older pre-release version as I tried seeing if a newer version caused a couple issues (it did not). I'll see if that's the cause (but I don't think so).
^ No change. We should probably discuss this elsewhere though.

e: one dark pro supports support.function.std.rust. It seems to just be up to the theme author to support it, but ra should support it itself everywhere

@blyxyas
Copy link
Member

blyxyas commented Jun 28, 2023

Just updated VSCode to the latest version, changed theme to One Dark Pro and reloaded RA. It works now.
... It's... it's probably still a good idea to make this change.

@Centri3
Copy link
Member Author

Centri3 commented Jun 28, 2023

Yeah I still agree adding a configuration option is a good idea

@bors
Copy link
Contributor

bors commented Jul 1, 2023

☔ The latest upstream changes (presumably #11020) made this pull request unmergeable. Please resolve the merge conflicts.

clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
book/src/lint_configuration.md Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_utils/src/check_proc_macro.rs Outdated Show resolved Hide resolved
clippy_utils/src/usage.rs Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jul 8, 2023

☔ The latest upstream changes (presumably #10788) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo Alexendoo added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jul 11, 2023
@blyxyas
Copy link
Member

blyxyas commented Jul 12, 2023

@rustbot label: -I-nominated

@rustbot rustbot removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jul 12, 2023
@bors
Copy link
Contributor

bors commented Jul 13, 2023

☔ The latest upstream changes (presumably #11095) made this pull request unmergeable. Please resolve the merge conflicts.

@Centri3 Centri3 force-pushed the absolute_path branch 2 times, most recently from f8a4ec2 to 38b871f Compare July 14, 2023 01:48
@bors
Copy link
Contributor

bors commented Jul 18, 2023

☔ The latest upstream changes (presumably #11140) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo
Copy link
Member

You can only pass flags to cargo test with the uitest alias because of how the others are defined

uitest = "test --test compile-test"
uibless = "test --test compile-test -- -- --bless"
bless = "test -- -- --bless"

@Centri3
Copy link
Member Author

Centri3 commented Jul 18, 2023

Interesting, I'll keep that in mind, though I'm not sure why BLESS didn't work then (I tried that as well)

Nevermind BLESS= works fine?? That would make sense if it worked before, I'm not sure what I was doing wrong before then

clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
clippy_lints/src/absolute_symbol_paths.rs Outdated Show resolved Hide resolved
@Alexendoo
Copy link
Member

Oh and remember to update the config value names if the lint name changes

@Centri3 Centri3 changed the title New lint [absolute_symbol_paths] New lint [absolute_paths] Jul 19, 2023
@Centri3 Centri3 force-pushed the absolute_path branch 5 times, most recently from c5e0a11 to c1028af Compare July 19, 2023 09:29
@bors
Copy link
Contributor

bors commented Jul 20, 2023

☔ The latest upstream changes (presumably #11107) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo
Copy link
Member

Thanks! r=me,blyxyas with squashed commits

@Centri3 Centri3 force-pushed the absolute_path branch 2 times, most recently from f8b418d to 8d23767 Compare July 21, 2023 21:57
Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

oop, actually a couple things but then it's good

The commit message calls it absolute_symbol_paths still also

CHANGELOG.md Outdated Show resolved Hide resolved
book/src/lint_configuration.md Outdated Show resolved Hide resolved
clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved
@Alexendoo
Copy link
Member

👍

@bors r=Alexendoo,blyxyas

@bors
Copy link
Contributor

bors commented Jul 21, 2023

📌 Commit 9cf1509 has been approved by Alexendoo,blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 21, 2023

⌛ Testing commit 9cf1509 with merge 58df1e6...

@bors
Copy link
Contributor

bors commented Jul 21, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo,blyxyas
Pushing 58df1e6 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new lint: absolute symbol path usage
5 participants