-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Clippy version to Clippy's lint list #7813
Conversation
I'm currently stuck on the attribute regex for The current status is (Solved, regex ignores spaces, the |
I don't think the Clippy version a lint was introduced in, is really helpful for Clippy users, since Did you think about adding an (optional) field to the macro instead of an attribute? What benefit does the attribute have? |
It'll be some work either way. Marking a bunch of lints with "pre" would actually be a big help. 🙃 I would suggest using Rust 1.29 as a cut-off point as that was also the first Rust version listed on the lint list version index. Setting the version to
The attribute enables the metadata collection lint to easily get the version, other possibilities would be to put it in a doc string or something. Having an attribute like this was straightforward to parse. I thought about adding an extra field to the macro and only adding the attribute inside the expansion. That would make lint description look like this: declare_clippy_lint! {
/// ### What it does
/// Collects metadata about clippy lints for the website.
///
pub INTERNAL_METADATA_COLLECTOR,
"pre 1.57.0",
internal_warn,
"A busy bee collection metadata about lints"
} I like the look with the attribute a bit more :) |
Yes, I would also make the cut off where we first started creating Clippy releases with every Rust release. Getting added lints per release could look somthing like this:
Shouldn't be hard to write such a script. Seeing this in the code, I don't think attribute vs in-macro makes much of a difference. Also as you wrote we may be able to reuse this to mark lints with |
That's an interesting idea, the JSON files can also be downloaded from the
That's what I was guessing, just wanted to throw that idea out there ^^
I think that our current setup would make it hard to have the nightly build have exclusive lints, as we reuse lint passes for several lints. My idea was to handle these with a new
That's just an idea, though 🙃 |
That can be done with just another script :)
I want to avoid to introduce a new lint group for nightly. I want that using Clippy on nightly vs on stable is exactly the same and you don't have to add anything to your code or CI configuration to use the new nightly lints. Just use Clippy from the nightly toolchain instead of stable and you get all the new nightly lints how they may end up in stable one day. I could imaging to include this in our new span_lint utils. So that we have a check, if the lint is in nightly and only emit it if nightly Clippy is run. |
Guess I'll do some scripting ^^. Should I push the commit on this branch and make the attribute mandatory?
True, that would be nicer! An idea to keep in mind. Are you okay moving this forward as it is, to display the version on the website? 🙃 |
Yes, but please keep it isolated in an extra commit.
Yes, this seems good to me |
Hey, I'm currently adjusting the |
9cf6f3b
to
05c2fa2
Compare
Why not use Also if we should reuse this for marking lints as nightly-only, we can just hardcode the string |
No real reason just haven't thought of that ^^
True, that would also be my plan later down the line, depending on how we're going to implement it. Until that is implemented I thought it would be good to automatically fill that field. But we can set it to |
Depends on how hard it is to fill it. But with cargp_metadata it should be doable in only a few lines of code. |
a03df14
to
90810cd
Compare
I was able to use |
3c032ae
to
cb321f4
Compare
This PR needs to be rebased to add another |
6ab38e9
to
a6dda40
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.
And I decided to write a script using nushell because why not? This was a mistake... I spend way more time on this than I would like to admit. It has definitely been more than 4 hours
4 hours is not too bad for this.
I would have written a few simple python scripts for this.
Since my suggestions may require some new scripts, let me know if you want me to write them or want some help writing them. (I can only help with python scripts, because I don't know anything about nushell)
I always underestimate the time a small change will take. A coworker once told me that it's usually a good idea to multiply the early estimated time for a task by 2. For me, this multiplier is about 5 times, as I always oversimplify stuff xD.
That would have probably been the smart move. Python is a nice scripting language, it has been some time since I used it, though. 😅
The adjustments should be pretty simple with some wizardry ✨. I can simply rename the
BTW. really understandable. Nushell is basically a console prototype that organizes data in tables and allows the definition of queries. The idea is fascinating IMO. However, it's still in its early stages and I mainly used it as a gimmick. It's still better to use a normal console or any other scripting language for most use cases. Still a fun learning experience. :) |
☔ The latest upstream changes (presumably #7853) made this pull request unmergeable. Please resolve the merge conflicts. |
5d0278d
to
f5702bc
Compare
I accidentally created another monster with 700+ changes. Sorry, it likes humans, don't worry xD |
I like the "Added in" version most. Thanks for providing the comparison! |
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 also noticed, that no lints were actually added in 1.29.0. I guess this is because 0.1.29 is the same version as 0.0.212. I don't think an action is required here.
Sorry to kill the fun in the comments 😛
So, some context for this, well, more a story. I'm not used to scripting, I've never really scripted anything, even if it's a valuable skill. I just never really needed it. Now, `@flip1995` correctly suggested using a script for this in `rust-clippy#7813`... And I decided to write a script using nushell because why not? This was a mistake... I spend way more time on this than I would like to admit. It has definitely been more than 4 hours. It shouldn't take that long, but me being new to scripting and nushell just wasn't a good mixture... Anyway, here is the script that creates another script which adds the versions. Fun... Just execute this on the `gh-pages` branch and the resulting `replacer.sh` in `clippy_lints` and it should all work. ```nu mv v0.0.212 rust-1.00.0; mv beta rust-1.57.0; mv master rust-1.58.0; let paths = (open ./rust-1.58.0/lints.json | select id id_span | flatten | select id path); let versions = ( ls | where name =~ "rust-" | select name | format {name}/lints.json | each { open $it | select id | insert version $it | str substring "5,11" version} | group-by id | rotate counter-clockwise id version | update version {get version | first 1} | flatten | select id version); $paths | each { |row| let version = ($versions | where id == ($row.id) | format {version}) let idu = ($row.id | str upcase) $"sed -i '0,/($idu),/{s/pub ($idu),/#[clippy::version = "($version)"]\n pub ($idu),/}' ($row.path)" } | str collect ";" | str find-replace --all '1.00.0' 'pre 1.29.0' | save "replacer.sh"; ``` And this still has some problems, but at this point I just want to be done -.-
a0a8acc
to
6f4221e
Compare
6f4221e
to
94bc0a1
Compare
Ups, good to know ^^. The difference shouldn't really matter :)
No problem ^^. I'm sometimes a bit unsure what is okay and what not, when it comes to these kinds of things. 🙃 It should be ready now :) |
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.
What happens when the version is not defined for a lint? Do we have an automated check, that makes sure that a version is defined for every lint?
No, we don't. I misunderstood you before and thought it shouldn't be mandatory. I'll add a new lint internal lint to make the attribute mandatory. That ensures nice error messages and enables us to allow it for internal lints 🙃 |
9435077
to
8c45fd8
Compare
Oh I think I said that before 😄 But now, I think it's better to just require it for every lint to avoid getting to a point where we mark some of our new lints when we remember to do so, but get more inconsistent over time. |
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.
LGTM. Let me quickly try to build the website locally, and this should be ready to merge.
@bors r+ Thanks! And sorry for taking so long for reviewing such a bit-rotty PR. (The next possible improvement for our website would be to add some more filters. E.g. filtering for version added, applicability, ... This will also involve making the existent filters more usable) |
📌 Commit 8c45fd8 has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
No worries 🙃, thanks for the review!
My next goal for the website was collecting renamed lints and create some kind of indicator, when somebody searches for the old name. Expanding on the filters is also a good idea, a copy-lint-name function would also be nice. Currently, I'm trying to wrap up my larger project before starting on my bachelor thesis in December. I'll create issues for the proposed improvements, these can also be labeled as good-first-issues 🙃 |
Hey, hey, the semester is finally over, and I wanted to get back into hacking on Clippy. It has also been some time since our metadata collection monster has been feed. So, this PR adds a new attribute
clippy::version
to document which version a lint was stabilized. I considered usinggit blame
but that would be very hacky and probably not accurate.I'm also thinking that this attribute can be used to have a
clippy::nightly
lint group which is allow-by-default that delays setting the actual lint group until the defined version is reached. Just something to consider regarding #6623 🙃This PR only adds the version to 4 lints to keep it reviewable. I'll do a followup PR to add the version to other lints if the implementation is accepted 🙃
Also, mobile approved xD
r? @flip1995
cc: #7172
closes: #6492
changelog: Clippy's lint list now displays the version a lint was added. 🎉
Example lint declaration after this update: