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

Metadata collection monster eating deprecated lints #7197

Merged
merged 2 commits into from
May 12, 2021

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented May 8, 2021

This adds the collection of deprecated lints to the metadata collection monster. The JSON output has the same structure with the new lint group "DEPRECATED". Here is one of fourteen examples it was able to dig up in Clippy's code:

  {
    "id": "assign_op_pattern",
    "id_span": {
      "path": "src/assign_ops.rs",
      "line": 34
    },
    "group": "clippy::style",
    "docs": " **What it does:** Checks for `a = a op b` or `a = b commutative_op a` patterns.\n\n **Why is this bad?** These can be written as the shorter `a op= b`.\n\n **Known problems:** While forbidden by the spec, `OpAssign` traits may have\n implementations that differ from the regular `Op` impl.\n\n **Example:**\n ```rust\n let mut a = 5;\n let b = 0;\n // ...\n // Bad\n a = a + b;\n\n // Good\n a += b;\n ```\n",
    "applicability": {
      "is_multi_part_suggestion": false,
      "applicability": "MachineApplicable"
    }
  }

And you! Yes you! Sir or Madam can get all of this for free in Clippy if this PR gets merged. (Sorry for the silliness ^^)


See: #7172 for the full metadata collection to-do list or to suggest a new feature in connection to it 🙃


changelog: none

r? @flip1995

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 8, 2021
@ghost
Copy link

ghost commented May 9, 2021

It's 'deprecated'; not 'depreciated'. We're talking APIs not financial statements.

@xFrednet
Copy link
Member Author

xFrednet commented May 9, 2021

Ups, I'll update the PR later today. Thank you 😅

@xFrednet xFrednet force-pushed the 4310-depreciated-lints-collection branch from 0fddc6d to d849e95 Compare May 9, 2021 19:59
@xFrednet xFrednet changed the title Metadata collection monster eating depreciated lints Metadata collection monster eating deprecated lints May 9, 2021
@xFrednet
Copy link
Member Author

xFrednet commented May 9, 2021

I've fixed the wrong vocab usage in the main commit itself. I don't think it was worth it to add a new one 🙃

clippy_lints/src/deprecated_lints.rs Outdated Show resolved Hide resolved
clippy_lints/src/lib.rs Show resolved Hide resolved
@xFrednet xFrednet force-pushed the 4310-depreciated-lints-collection branch from 97ee471 to d7a532c Compare May 10, 2021 22:21
@xFrednet xFrednet requested a review from flip1995 May 10, 2021 22:23
@xFrednet
Copy link
Member Author

It seems like I'll need to update clippy_dev as well. But that's a TODO for tomorrow 🙃

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Hm, now that I see the effect of this, I think having a more detailed documentation in addition to the reason displayed in the error message is a better approach. Otherwise we would dump a multi line error message if someone uses a deprecated lint.

Before you start working on clippy_dev, just revert this to the version you had before my silly idea and lets revisit this later. Sorry about that!

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member Author

Yes, I had the same gut feeling during the implementation. Documenting deprecated lints in their own module makes them more approachable to newcomers as well due to the reduced scope, IMO. Anyways, This implementation will be kept in a backup branch in case we need it at a later point in time. I'll remove the commits from this branch.

Was there anything else I should update in the original implementation?

@flip1995
Copy link
Member

Was there anything else I should update in the original implementation?

Nope, the original implementation looked good.

@xFrednet xFrednet force-pushed the 4310-depreciated-lints-collection branch from d7a532c to 4a1da74 Compare May 11, 2021 16:33
@xFrednet xFrednet force-pushed the 4310-depreciated-lints-collection branch from 4a1da74 to a988a90 Compare May 11, 2021 16:33
@xFrednet
Copy link
Member Author

Okay, I've removed the commits to collect the deprecation reasoning from store.register_removed. A backup is in a second branch with a link to it in the tracking issue. So, that should be it IMO.

@flip1995
Copy link
Member

@bors r+

Thanks! (And sorry for the back and forth)

@bors
Copy link
Collaborator

bors commented May 12, 2021

📌 Commit a988a90 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented May 12, 2021

⌛ Testing commit a988a90 with merge aa15a54...

@xFrednet
Copy link
Member Author

No worries 🙃, and thanks for the quick review cycle. I'll create the configuration collection PR later today and that should be it when it comes to the new collection itself. The next step it then to adapt the CI and website. I'm also looking forward to work into a new topic after that xD

@bors
Copy link
Collaborator

bors commented May 12, 2021

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

@bors bors merged commit aa15a54 into rust-lang:master May 12, 2021
@xFrednet xFrednet deleted the 4310-depreciated-lints-collection branch July 28, 2021 16:42
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.

4 participants