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

Move lints configuration to manifest #822

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

filnet
Copy link
Contributor

@filnet filnet commented Nov 18, 2023

This is possible since Rust 1.74.0.

This PR shows what it looks like.

ash/Cargo.toml Outdated

[lints.clippy]
use_self = "warn"
too_many_arguments = "allow"
Copy link
Contributor Author

@filnet filnet Nov 18, 2023

Choose a reason for hiding this comment

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

Unfortunately it is not possible to override lints from the root.
So here the common lints are duplicated to be able to add the three "allow"

Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

I am intentionally not pushing this to the ash repo because:

  1. Lints are explicitly inherited from the workspace, not implicitly forced down;
  2. Unnecessary MSRV bump;
  3. Strange duplication in overrides.

Can I close this as "Not planned"?

@filnet
Copy link
Contributor Author

filnet commented Nov 18, 2023

Can I close this as "Not planned"?

No problems...

@filnet
Copy link
Contributor Author

filnet commented Nov 19, 2023

I also found it strange that workspace lints can't be overridden at the workspace member level.
This behavior is not documented.

@filnet
Copy link
Contributor Author

filnet commented Nov 19, 2023

I dropped the MSRV bump.

What about the lint fixes and the "resolve" warning fix ? Should we keep them ?

@filnet
Copy link
Contributor Author

filnet commented Nov 19, 2023

I moved the ash package specific lints back to the lib.rs file and enabled the workspace lints.
This works as "expected".

PS: I don't expect this to be merged. Just showcasing.

@filnet filnet force-pushed the clippy branch 3 times, most recently from e33abe0 to 0bb826e Compare December 6, 2023 15:04
@MarijnS95
Copy link
Collaborator

PS: I don't expect this to be merged. Just showcasing.

Note that everyone gets notifications when you're debugging by means of (force-)pushing.

@filnet
Copy link
Contributor Author

filnet commented Dec 6, 2023

Note that everyone gets notifications when you're debugging by means of (force-)pushing.

Sorry about that. I'll be more careful next time.

@filnet
Copy link
Contributor Author

filnet commented Dec 6, 2023

Current status / limitations:

  • all common lints are now configured at the workspace level.
  • ash-window now uses the common lints (was using a subset).
  • as mentioned earlier, it is not possible to override or tweak the lints at the workspace member level (but it is still possible to do it from main.rs/lib.rs).

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.

2 participants