Skip to content

[flake8-pyi] Implement PYI057#11486

Merged
AlexWaygood merged 10 commits intoastral-sh:mainfrom
tomasr8:pyi057
May 29, 2024
Merged

[flake8-pyi] Implement PYI057#11486
AlexWaygood merged 10 commits intoastral-sh:mainfrom
tomasr8:pyi057

Conversation

@tomasr8
Copy link
Copy Markdown
Contributor

@tomasr8 tomasr8 commented May 21, 2024

Summary

Adds Y057 from flake8-pyi.

I don't think we can apply any autofix here?

I'm new to rust so apologies in advance if I've made some rookie mistakes 😅

Test Plan

cargo test + cargo insta review

@tomasr8 tomasr8 requested a review from AlexWaygood as a code owner May 21, 2024 21:56
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+1 -0 violations, +0 -0 fixes in 1 projects; 49 projects unchanged)

python/typeshed (+1 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select E,F,FA,I,PYI,RUF,UP,W

+ stdlib/_collections_abc.pyi:10:5: PYI057 Do not use `typing.ByteString`, which has unclear semantics and is deprecated

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PYI057 1 1 0 0 0

@zanieb
Copy link
Copy Markdown
Member

zanieb commented May 21, 2024

Hi! The primary reviewer for this is out right now so it might be a bit slower than usual to get reviewed.

Could you change the rule to be in Preview instead of the Stable group?

Additionally, I think use of resolve_qualified_name might be helpful in your implementation:

ruff/crates/ruff_python_semantic/src/model.rs

Lines 726 to 741 in f79c980

/// Resolves the [`Expr`] to a fully-qualified symbol-name, if `value` resolves to an imported
/// or builtin symbol.
///
/// E.g., given:
///
///
/// ```python
/// from sys import version_info as python_version
/// print(python_version)
/// ```
///
/// ...then `resolve_qualified_name(${python_version})` will resolve to `sys.version_info`.
pub fn resolve_qualified_name<'name, 'expr: 'name>(
&self,
value: &'expr Expr,
) -> Option<QualifiedName<'name>>

@zanieb
Copy link
Copy Markdown
Member

zanieb commented May 21, 2024

Welcome to the project :)

@tomasr8
Copy link
Copy Markdown
Contributor Author

tomasr8 commented May 21, 2024

Additionally, I think use of resolve_qualified_name might be helpful in your implementation:

I thought there might be something like this already, but couldn't find it. Thanks!

AlexWaygood
AlexWaygood reviewed May 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great to see you around here @tomasr8! :-D Overall this looks great.

Comment thread crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs Outdated
Comment thread crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs Outdated
Comment thread crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs
Comment thread crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs
@tomasr8
Copy link
Copy Markdown
Contributor Author

tomasr8 commented May 29, 2024

oops didn't notice you already fixed the conflict 😅

@AlexWaygood
Copy link
Copy Markdown
Member

AlexWaygood commented May 29, 2024

hehe, no worries

@tomasr8
Copy link
Copy Markdown
Contributor Author

tomasr8 commented May 29, 2024

Thanks for the review! (as always :D)

I removed the fix_title implementation and used an enum instead of the full_name string as you suggested.

The bytestring_import function should also be a bit more efficient now since I check for the module id before entering the loop and return early if it doesn't match.

AlexWaygood
AlexWaygood approved these changes May 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks very much! Looks great now. I just pushed a couple of nitpick-fixes to your branch :-)

@AlexWaygood AlexWaygood added rule Implementing or modifying a lint rule preview Related to preview mode features labels May 29, 2024
AlexWaygood
AlexWaygood reviewed May 29, 2024
View reviewed changes
Comment thread crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs
Comment on lines +62 to +68
let semantic = checker.semantic();
if !semantic
.seen
.intersects(Modules::TYPING | Modules::COLLECTIONS)
{
return;
}
Copy link
Copy Markdown
Member

@AlexWaygood AlexWaygood May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tomasr8: I added this in my latest push. It's an optimisation we do in a lot of rules. It's a very cheap check to see whether any of the relevant modules we care about have been imported at all. If not, we can just short-circuit the rest of the rule :-)

Copy link
Copy Markdown
Contributor Author

@tomasr8 tomasr8 May 29, 2024

Choose a reason for hiding this comment

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

Ah cool! I'll keep that in mind for future PRs :)

@AlexWaygood AlexWaygood enabled auto-merge (squash) May 29, 2024 10:03
@AlexWaygood AlexWaygood merged commit 7659114 into astral-sh:main May 29, 2024
@tomasr8 tomasr8 deleted the pyi057 branch May 29, 2024 10:07
@BrewTestBot BrewTestBot mentioned this pull request May 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Assignees

No one assigned

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tomasr8 @zanieb @AlexWaygood