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

Fixes #9677 - Lint against mod lib; #9708

Closed
wants to merge 4 commits into from

Conversation

st3fan
Copy link

@st3fan st3fan commented Oct 25, 2022

Warning: Noob in action. This is my first attempt to work on #9677 - work in progress. There is a lot to fix and clean up here.

--

Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
changelog: none. Otherwise, please write a short comment
explaining your change.

It's also helpful for us that the lint name is put within backticks (` `),
and then encapsulated by square brackets ([]), for example:

changelog: [`lint_name`]: your change

If your PR fixes an issue, you can add fixes #issue_number into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

  • Followed lint naming conventions
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.


Please write a short comment explaining your change (or "none" for internal only changes)

changelog:

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 25, 2022
}
declare_lint_pass!(ModLib => [MOD_LIB]);

impl EarlyLintPass for ModLib {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, thanks for working on this! I know this PR isn’t ready for review, just wanted to say we’re trying to make all new lints to use ‘LateLintPass’, cheers! Feel free to share any of your issues

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @kraktus I've updated the code to implement LateLintPass instead. Am I going in the right direction here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does! Looks like your lint is not registered yet, using cargo new_lint (don’t remember the exact name of the subcommand, find it with cargo —help while in clippy repo, will create the skeleton of the lint for you

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually cargo dev update_lint should suffice in the current situation. The lint exists, but it isn't yet registered.

#![allow(unused)]
#![warn(clippy::mod_lib)]

mod lib;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a mod lib { } should suffice here, which obviates the need for a secondary file.

@bors
Copy link
Collaborator

bors commented Oct 31, 2022

☔ The latest upstream changes (presumably #9506) made this pull request unmergeable. Please resolve the merge conflicts.

@llogiq
Copy link
Contributor

llogiq commented Nov 6, 2022

Ping! Do you still plan working on this? I think with a rebase and running cargo dev update_lints this might mostly be mergeworthy.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 6, 2022
@Alexendoo
Copy link
Member

cross-posting #9677 (comment)

@llogiq llogiq closed this Nov 9, 2022
@st3fan st3fan deleted the st3fan/mod-lib branch November 11, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants