rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

Fix/12035 silence struct field names

Open alexis-langlet opened this issue 5 months ago • 12 comments

fixes #12035

Allows to silence lints struct_field_names and enum_variant_names through their impls. This is done in order for some crates such as serde to silence those lints

changelog: [struct_field_names]: allow to silence lint through struct's impl changelog: [enum_variant_names]: allow to silence lint through enum's impl

alexis-langlet avatar Jan 23 '24 08:01 alexis-langlet

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot avatar Jan 23 '24 08:01 rustbot

@bors r+

Manishearth avatar Jan 23 '24 19:01 Manishearth

:pushpin: Commit 9a3cdbe4adf6614df2202307b1b5255332e1de45 has been approved by Manishearth

It is now in the queue for this repository.

bors avatar Jan 23 '24 19:01 bors

:hourglass: Testing commit 9a3cdbe4adf6614df2202307b1b5255332e1de45 with merge 1ffad3acf864211bc81a61217f75758e22c99170...

bors avatar Jan 23 '24 19:01 bors

:broken_heart: Test failed - checks-action_test

bors avatar Jan 23 '24 20:01 bors

:hourglass: Testing commit 9a3cdbe4adf6614df2202307b1b5255332e1de45 with merge 8a2c2a2518bb0f967f667c7be1a80c2c25450ee7...

bors avatar Feb 28 '24 15:02 bors

:broken_heart: Test failed - checks-action_dev_test

bors avatar Feb 28 '24 15:02 bors

Hey @y21, this is a ping from triage. It looks like this PR was already approved. Is there anything that needs to be done, or did bors just fail for bors reasons?

xFrednet avatar Apr 01 '24 10:04 xFrednet

Hi @xFrednet, after discussing a bit about the related issue on Zulip, it looks like it won't really be useful. So maybe we can close this PR and the related issue?

alexis-langlet avatar Apr 02 '24 16:04 alexis-langlet

Hey @alexis-langlet, I've checked the tests and agree to close the issues. It seems a bit weird to me that impls should allow it, since the lints should trigger on the enum definition and not the impls.

Thank you for working on this and also being part of the discussion.

xFrednet avatar Apr 04 '24 09:04 xFrednet

Okay, after reading skimming through the issue, I understand the motivation for allowing this lint to be allowed on an impl. My suggestion might be to allow traits to allow this lint instead, but it all seems a bit weird. I would like to get more input from the others.

I'll nominate this for the next meeting with the question:

  • Do we want to allow this lint to be allowed on impl items or maybe trait definitions?

@rustbot label +I-nominate

Personally, I think it's better to keep this lint simple. For a user it should be easy to add #[allow(..)] to the enum. But since others already approved this PR, it's better to discuss this IMO

xFrednet avatar Apr 04 '24 09:04 xFrednet

:umbrella: The latest upstream changes (presumably #12635) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Apr 12 '24 19:04 bors