Skip to content

Move lints configuration to manifest#822

Draft
filnet wants to merge 3 commits into
ash-rs:masterfrom
filnet:clippy
Draft

Move lints configuration to manifest#822
filnet wants to merge 3 commits into
ash-rs:masterfrom
filnet:clippy

Conversation

@filnet

@filnet filnet commented Nov 18, 2023

Copy link
Copy Markdown
Contributor

This is possible since Rust 1.74.0.

This PR shows what it looks like.

Comment thread ash/Cargo.toml Outdated

[lints.clippy]
use_self = "warn"
too_many_arguments = "allow"

@filnet filnet Nov 18, 2023

Copy link
Copy Markdown
Contributor Author

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"

@MarijnS95 MarijnS95 left a comment

Copy link
Copy Markdown
Collaborator

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

filnet commented Nov 18, 2023

Copy link
Copy Markdown
Contributor Author

Can I close this as "Not planned"?

No problems...

@filnet

filnet commented Nov 19, 2023

Copy link
Copy Markdown
Contributor Author

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

@filnet

filnet commented Nov 19, 2023

Copy link
Copy Markdown
Contributor Author

I dropped the MSRV bump.

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

@filnet

filnet commented Nov 19, 2023

Copy link
Copy Markdown
Contributor Author

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 marked this pull request as draft December 6, 2023 14:48
@filnet filnet force-pushed the clippy branch 3 times, most recently from e33abe0 to 0bb826e Compare December 6, 2023 15:04
@MarijnS95

Copy link
Copy Markdown
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

filnet commented Dec 6, 2023

Copy link
Copy Markdown
Contributor Author

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

filnet commented Dec 6, 2023

Copy link
Copy Markdown
Contributor Author

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