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

Prune the canonical lint set #88

Merged
merged 2 commits into from
Jan 31, 2025
Merged

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Jan 29, 2025

We've had complaints that the lint set is too strict, which I think is fair. As such, I suggest that we remove some of the most pedantic lints. It also adds a new set which we "want to" follow, but which don't block PRs.
It's expected that we would work to not regress these between (for example) crate releases.

Of note:

  1. allow_attributes is removed from the default set. This doesn't mean we shouldn't still aim to use expect, but having an allow is fine.
  2. default_trait_access is newly added to the new "periodic" category.
  3. missing_panics_doc is removed entirely.
  4. wildcard_imports is removed entirely

We've had complaints that the lint set is too strict, which
I think is fair.
As such, some of the most

Of note:
1) `allow_attributes` is removed from the default set.  This doesn't mean we shouldn't still aim to use `expect`, but having an allow is fine.
2) `default_trait_access` is *added* to the new "periodic" category.
3) `missing_panics_doc` is removed entrely.
4) `wildcard_imports` is removed entirely
Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Looks good.

I think removing match_same_arms from the default set is particularly good. It's a good lint some times, but we also have existing code where it clearly makes things worse.

@xStrom
Copy link
Member

xStrom commented Jan 30, 2025

On further thought, are there examples of issues with clippy::doc_markdown? I haven't looked deeply into it, but from memory it has been a good experience.

@waywardmonkeys you've been the biggest implementor of this. What's your take, should we keep it on by default?

@waywardmonkeys
Copy link
Contributor

I think clippy::doc_markdown should stay on by default.

@waywardmonkeys
Copy link
Contributor

We can also have CI run a job periodically (every Monday morning?) that runs the periodic checks.

@DJMcNab
Copy link
Member Author

DJMcNab commented Jan 30, 2025

I removed doc_markdown as:

  1. it strikes me as the sort of thing which someone running into it in CI would find extremely annoying
  2. It's really simple to do drive-by fixes for (hence being in the editor category)

Certainly happy to optimistically restore it, though.

A weekly check with a Zulip webhook message for the periodic checks might be nice. I suspect that's too much effort for the amount of gain (i.e. that re-adding them to CI proper would probably be better than that...), but I could be convinced.

@DJMcNab DJMcNab merged commit b85ce79 into linebender:main Jan 31, 2025
1 check passed
github-merge-queue bot pushed a commit to linebender/vello that referenced this pull request Feb 10, 2025
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Feb 17, 2025
See linebender/linebender.github.io#88 (and some other earlier PRs)

A follow up like linebender/vello#806 will also
be needed, but that can come later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants