Skip to content

Collect renamed lints #8843

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

Merged
merged 1 commit into from
Jun 4, 2022
Merged

Collect renamed lints #8843

merged 1 commit into from
Jun 4, 2022

Conversation

Serial-ATA
Copy link
Contributor

@Serial-ATA Serial-ATA commented May 19, 2022

changelog: Display past names of renamed lints on Clippy's lint list

cc #7172

r? @xFrednet

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 19, 2022
@xFrednet
Copy link
Member

xFrednet commented May 21, 2022

Have you thought about how we want to display the renamed lint name on the website?

I was considering something like this:

image

Or maybe

image

(Though I like the first option more)

In either case, we can prepare do some work in our metadata collector to make the website faster. That's why I'm asking 🙃


r? @dswij You might want to wait until we're resolved the presentation part. You're welcome to also share your ideas 🙃

@Serial-ATA
Copy link
Contributor Author

I had a similar idea to the first one, except it was "Past names" instead of "Notes". Since it'll show up in search regardless, I don't think making a new entry to point to the rename is necessary. 🙂

@xFrednet
Copy link
Member

I had a similar idea to the first one, except it was "Past names" instead of "Notes". Since it'll show up in search regardless, I don't think making a new entry to point to the rename is necessary.

Past names is a better name. In that case, I would already add this to the documentation in rust, similar to how it's done for configurations. 🙃

@dswij
Copy link
Member

dswij commented May 23, 2022

Was just thinking if Notes can even include recent changelogs to the lint, but that's probably too much noise

What happens when we rename a lint twice? Would Past Names have all past lint names?

@Serial-ATA
Copy link
Contributor Author

I updated it to account for multiple renames 🙂.

@xFrednet
Copy link
Member

Nice, this still leaves the question where these will be rendered. I'm still for adding it to the lint description like it's done for the configuration 🙃

@Serial-ATA
Copy link
Contributor Author

Yeah, I think it should go under the configuration as a bulleted list.

@xFrednet
Copy link
Member

xFrednet commented May 25, 2022

What I meant with my question of where this will be rendered was, where in our tech-stack. So if this will append the section in the metadata collector (Like the configuration section) or on the website. Where possible, I usually prefer adding it during the collection in rust, since any logic on Clippy's lint list is multiplied by the number of lints and delays the rendering. That's also why I'm mentioning this here and not in the related frontend PR since, we can implement this without touching the frontend. 🙃

Sorry, if that was unclear 😅

@Serial-ATA
Copy link
Contributor Author

Oh 😄. In that case, yeah it should definitely be added like the configuration. I'll get that done soon.

@Serial-ATA Serial-ATA force-pushed the collect-renamed branch 2 times, most recently from 2cec2ad to 0db5e6b Compare May 25, 2022 21:27
@Serial-ATA
Copy link
Contributor Author

Here's how it looks:
example

@xFrednet
Copy link
Member

The example looks good IMO 👍

@flip1995
Copy link
Member

The Past Names option also was my first thought when seeing this PR in my notifications 👍

@xFrednet
Copy link
Member

xFrednet commented Jun 1, 2022

I'm stealing this review back. I hope that's alright with you @dswij

r? @xFrednet


I should get to this during the weekend 🙃

@dswij
Copy link
Member

dswij commented Jun 2, 2022

Sure, go ahead!

@xFrednet
Copy link
Member

xFrednet commented Jun 4, 2022

Excellent work! I've also tested it locally and everything works perfectly. Thank you very much for this update. I'm happy to see the metadata collection project to make progress and with your work probably also be completed! ❤️ This was my first big project in Clippy, and it's also the one that held me here and introduced me to the team. I hope you also had fun with it.

Now enough rambling:

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2022

📌 Commit 45be175 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Jun 4, 2022

⌛ Testing commit 45be175 with merge c7893da...

bors added a commit that referenced this pull request Jun 4, 2022
Collect renamed lints

changelog: none
cc #7172

r? `@xFrednet`
@xFrednet
Copy link
Member

xFrednet commented Jun 4, 2022

@bors r-

@xFrednet
Copy link
Member

xFrednet commented Jun 4, 2022

Sorry for the r-, this is worth a changelog entry. I don't want this to slip under the radar of who ever will write it for that version. I've added the entry and also ticked of the checkbox in the tracking issue

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2022

📌 Commit 45be175 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Jun 4, 2022

⌛ Testing commit 45be175 with merge 8ef4908...

@bors
Copy link
Contributor

bors commented Jun 4, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 8ef4908 to master...

@bors bors merged commit 8ef4908 into rust-lang:master Jun 4, 2022
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.

6 participants