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

lints: Enable warn(unused_qualifications) #53

Merged

Conversation

waywardmonkeys
Copy link
Contributor

This helps point out where the code can be more concistent with an identifier already being in scope and not needing qualifications.

@waywardmonkeys waywardmonkeys requested a review from dfrg May 20, 2024 13:40
@waywardmonkeys waywardmonkeys force-pushed the fix-unused-qualifications branch from 3fabe01 to 9edcfa5 Compare May 20, 2024 13:43
node: Node,
config_file_path: impl AsRef<std::path::Path>,
) -> Option<std::path::PathBuf> {
fn resolve_dir(node: Node, config_file_path: impl AsRef<Path>) -> Option<std::path::PathBuf> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a consistency argument to make for this remaining as std::path::Path and the other usages of that to do the same and remove the use std::path::Path since all other std stuff comes in via the fully qualified name.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we probably ought to go the other way and have use imports for everything in std (unless there's a name clash)

@waywardmonkeys
Copy link
Contributor Author

This is similar to one that is in Peniko: linebender/peniko#33 and another in Kurbo: linebender/kurbo#346

Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

Seems fine to me

node: Node,
config_file_path: impl AsRef<std::path::Path>,
) -> Option<std::path::PathBuf> {
fn resolve_dir(node: Node, config_file_path: impl AsRef<Path>) -> Option<std::path::PathBuf> {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we probably ought to go the other way and have use imports for everything in std (unless there's a name clash)

This helps point out where the code can be more concistent with an
identifier already being in scope and not needing qualifications.
@waywardmonkeys waywardmonkeys force-pushed the fix-unused-qualifications branch from 9edcfa5 to 839f7e7 Compare May 23, 2024 05:31
@waywardmonkeys waywardmonkeys added this pull request to the merge queue May 23, 2024
Merged via the queue into linebender:main with commit 4d3273d May 23, 2024
19 checks passed
@waywardmonkeys waywardmonkeys deleted the fix-unused-qualifications branch May 23, 2024 06:02
@@ -7,6 +7,7 @@ members = [
[workspace.package]
edition = "2021"
# Keep in sync with RUST_MIN_VER in .github/workflows/ci.yml and with the relevant README.md files.
# TODO: When this hits 1.74, move lint configuration into this file via a lints table.
Copy link
Member

@xStrom xStrom May 23, 2024

Choose a reason for hiding this comment

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

We don't need to wait for the MSRV to be that high to use the lints table. The MSRV cargo check commands won't fail, they'll just warn that there is an unused manifest key but the CI will still pass. The stable toolchain will do the actual linting.

This won't be a problem for projects higher up the stack either, as Cargo doesn't complain about unused manifest keys of dependencies.

Opened #61 to resolve this.

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.

3 participants