Skip to content

Enable and fix unused_qualifications lint #7715

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

Merged
merged 2 commits into from
Apr 10, 2025

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Apr 9, 2025

Improve code readability. What's even more important is that the same identifier is used identically everywhere in one file - i.e. chrono::DateTime would not be used with a fully-qualified name in one place, but with just DateTime in another.

@sylvestre
Copy link
Contributor

I am not convinced that it improves code readability. I think that for some cases, it decreases it

@nyurik nyurik force-pushed the unused_qualifications branch from 6c1be4d to 1e7411f Compare April 9, 2025 16:54
@nyurik
Copy link
Contributor Author

nyurik commented Apr 9, 2025

@sylvestre I agree that it some cases it makes things less readable -- but in that case the more explicit namespaced identifier should be used throughout the file. Otherwise you have a confusing situation - the same ident is used with and without the namespace just a few lines apart. This lint does not force shorter idents, it forces them to be consistently used per file.

Copy link

github-actions bot commented Apr 9, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@nyurik nyurik force-pushed the unused_qualifications branch 2 times, most recently from 13e4918 to 28f5b33 Compare April 9, 2025 20:43
Copy link

github-actions bot commented Apr 9, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@@ -16,9 +16,9 @@ use crate::os_str_to_c_string;

#[derive(Debug)]
pub(crate) struct FTS {
fts: ptr::NonNull<fts_sys::FTS>,
Copy link
Contributor

Choose a reason for hiding this comment

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

no a fan of this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sylvestre you had a few NonNull directly used - i updated those

@nyurik nyurik force-pushed the unused_qualifications branch 2 times, most recently from 39f893a to e796718 Compare April 10, 2025 20:59
@nyurik nyurik force-pushed the unused_qualifications branch from e796718 to 76b1b68 Compare April 10, 2025 21:13
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre merged commit 5bfbc30 into uutils:main Apr 10, 2025
69 checks passed
@nyurik nyurik deleted the unused_qualifications branch April 10, 2025 22:15
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