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: path_ends_with_ext #11483

Merged
merged 1 commit into from Sep 15, 2023
Merged

new lint: path_ends_with_ext #11483

merged 1 commit into from Sep 15, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Sep 11, 2023

Closes #11479

Not sure if it needs more test cases. I couldn't come up with any other ones, but it is a pretty simple lint logic wise with not too many checks

changelog: new lint: [path_ends_with_ext]

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 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 Sep 11, 2023
@Alexendoo
Copy link
Member

This would have a fair few false positives with stuff like .git or any other dotfile type things

If we can source one shipping a big list of file extensions would probably be the ideal solution since it wouldn't take up all that much space, the problem being how easily it is to source such a list

@y21
Copy link
Member Author

y21 commented Sep 11, 2023

Maybe a configuration option for additional dotfiles to allow would also be useful.

@kornelski
Copy link
Contributor

I suggest limiting it to 2-char and 3-char extensions. The longer the name, the more likely it is a dotfile.

I don't have any 2-char dotfiles. The 3-letter dotfiles I could find:

.git .svn .gem .npm .vim .env .rnd .ssh .vnc

@y21
Copy link
Member Author

y21 commented Sep 12, 2023

I suggest limiting it to 2-char and 3-char extensions. The longer the name, the more likely it is a dotfile.

I don't have any 2-char dotfiles. The 3-letter dotfiles I could find:

.git .svn .gem .npm .vim .env .rnd .ssh .vnc

That sounds great, I implemented this in the last commit (that and the ability to add custom dotfiles that clippy shouldn't warn).

clippy_lints/src/methods/path_ends_with_ext.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/path_ends_with_ext.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved
tests/ui/path_ends_with_ext.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@jdh8
Copy link

jdh8 commented Sep 13, 2023

How is this different from #6425 🧐

@y21
Copy link
Member Author

y21 commented Sep 13, 2023

How is this different from #6425 🧐

The description and code looks like it has a little bit of overlap and sounds similar, but that one seems to look for a different kind of suspicious pattern and I would say this one is different enough to be its own lint.

This lint looks for Path::ends_with calls and checks if the argument looks like a file extension and warns the user that this isn't what Path::ends_with does, which has nothing to do with case sensitivity like the other lint IMO.
The other lint looks for str::ends_with, which at least does the right thing in that it actually checks the extension (just not case insensitive).

We may want to change the suggestion to use str::eq_ignore_ascii_case instead of == though.
(And maybe that other lint could be improved to also flag the suggestion that we're currently making here: path.extension() == "rs")

bors added a commit that referenced this pull request Sep 14, 2023
…hearth

Truncate files when opening in metadata-collector

Fixes the issue seen here #11483 (comment) and in a couple other PRs

The changelog file was opened without truncating it, so if the new version is shorter than the old one stray contents would remain at the end of the file

The other two files first removed the file so didn't have this problem, but in all cases we now use `fs::write`/`File::create` which is write + create + truncate

changelog: none
@Alexendoo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 15, 2023

📌 Commit 981e960 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 15, 2023

⌛ Testing commit 981e960 with merge daadab5...

@bors
Copy link
Collaborator

bors commented Sep 15, 2023

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

@bors bors merged commit daadab5 into rust-lang:master Sep 15, 2023
8 checks passed
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.

path.ends_with(".ext") won't match file extension
6 participants