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

Clippy fixes #1129

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Clippy fixes #1129

wants to merge 10 commits into from

Conversation

chris-ha458
Copy link

Currently, the default cargo clippy shows a lot of potential fixes.
I've prepared a potential initial batch that could be applied.
There are more, and if this PR merges I shall prepare them further.

Most are one liner changes employing clippy suggestions with adjustments as necessary.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

I don't usually run Clippy because there's a lot I disagree with, and just don't have the time to sit down and configure the lint list to reflect my taste. (And also don't like littering code with allow(..) annotations on a case-by-case basis.)

With that said, I actually agree with most of these. There are a few that I would like to see reverted, but most look good. Part of the reason there are a lot is because regex-automata went through several rather large revisions while I worked on it, and not all code got thoroughly refactored. (That explains things like superfluous ref bindings and unnecessary clone calls.) And then of course, regex-automata's MSRV was much lower than what it is now compared to when I started.

regex-automata/src/nfa/thompson/range_trie.rs Outdated Show resolved Hide resolved
regex-automata/src/util/determinize/state.rs Outdated Show resolved Hide resolved
regex-automata/src/util/escape.rs Outdated Show resolved Hide resolved
@chris-ha458
Copy link
Author

I don't usually run Clippy because there's a lot I disagree with, and just don't have the time to sit down and configure the lint list to reflect my taste (And also don't like littering code with allow(..) annotations on a case-by-case basis.)

Hopefully 1.74's new change regarding cargo based lint configurations might change your mind.
I'm not sure if it is just tooling so can be done without changing MSRV, but if this is something you'd like to see, I'll try to implement it.
Hopefully, we can do this once and it will apply to all or most of your other crates as well.
Yeah you shared this aversion to littering code with lint annotations on a different crate and I was trying to be mindful of that.

If this is accepted I'll work on the others while being mindful of your comments here as well.

@BurntSushi
Copy link
Member

BurntSushi commented Nov 22, 2023

The new support in Cargo is great, but it doesn't really change my calculus here. And indeed, at least for a lot of my crates, we'd need to be mindful of MSRV. (It's possible it won't matter, for example, if Cargo will just ignore that section otherwise.) Basically, you still have to go through and figure out which lints to use and then go and apply them to all of the projects. I would have had to do that before and I would still have to do that today.

I'll try to be a bit clearer here... I personally do not want to have several dozen PRs scattered throughout all of my projects for setting up lint configurations right now. In part because I don't actually know whether I'll go through with it, so I don't want you to spend all that effort on it. Namely, the key thing that can't be addressed by any amount of work or configuration is that Clippy will invariably lead to adding allow(..) annotations in the source code. I can abide a couple here and there, but it's just in general not something I really want to put up with.

If my projects had a lot more contributors or multiple active maintainers then I would very likely feel differently about this.

@chris-ha458
Copy link
Author

I understand what you are saying.

I have reverted the issues you mentioned in this PR, are there any further changes you'd like to see?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants