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

add autofix for PYI055 #7886

Merged
merged 5 commits into from
Oct 13, 2023
Merged

add autofix for PYI055 #7886

merged 5 commits into from
Oct 13, 2023

Conversation

diceroll123
Copy link
Contributor

Summary

Adds autofix for PYI055

Questions:

  • is it intended that only the code within the def func(): gets fixed? 🤔 I feel like some of the top-level variables would also be affected.
  • should w: builtins.type[int] | builtins.type[str] | builtins.type[complex] turn into w: builtins.type[int | str | complex]?
    • or w: type[int | str | complex]?

Test Plan

cargo test, and manually.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 10, 2023

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+6, -6, 0 error(s))

airflow (+4, -4)

- [*] 16066 fixable with the `--fix` option (6343 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ [*] 16069 fixable with the `--fix` option (6343 hidden fixes can be enabled with the `--unsafe-fixes` option).
- airflow/providers/google/cloud/hooks/bigquery.py:1580:35: PYI055 Multiple `type` members in a union. Combine them into one, e.g., `type[CopyJob | QueryJob | LoadJob | ExtractJob]`.
+ airflow/providers/google/cloud/hooks/bigquery.py:1580:35: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[CopyJob | QueryJob | LoadJob | ExtractJob]`.
- airflow/providers/google/cloud/hooks/bigquery.py:1587:14: PYI055 Multiple `type` members in a union. Combine them into one, e.g., `type[CopyJob | QueryJob | LoadJob | ExtractJob]`.
+ airflow/providers/google/cloud/hooks/bigquery.py:1587:14: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[CopyJob | QueryJob | LoadJob | ExtractJob]`.
- airflow/providers/smtp/hooks/smtp.py:103:15: PYI055 Multiple `type` members in a union. Combine them into one, e.g., `type[smtplib.SMTP_SSL | smtplib.SMTP]`.
+ airflow/providers/smtp/hooks/smtp.py:103:15: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[smtplib.SMTP_SSL | smtplib.SMTP]`.

bokeh (+2, -2)

- [*] 17800 fixable with the `--fix` option (4727 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ [*] 17801 fixable with the `--fix` option (4727 hidden fixes can be enabled with the `--unsafe-fixes` option).
- src/bokeh/core/validation/decorators.py:67:13: PYI055 Multiple `type` members in a union. Combine them into one, e.g., `type[Error | Warning]`.
+ src/bokeh/core/validation/decorators.py:67:13: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[Error | Warning]`.

Rules changed: 1
Rule Changes Additions Removals
PYI055 8 4 4

@diceroll123 diceroll123 marked this pull request as ready for review October 11, 2023 03:50
Comment on lines 130 to 163
checker
.generator()
.expr(&Expr::Subscript(ast::ExprSubscript {
value: Box::new(Expr::Name(ast::ExprName {
id: "type".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
})),
slice: Box::new(Expr::Subscript(ast::ExprSubscript {
value: Box::new(Expr::Name(ast::ExprName {
id: "Union".into(),
ctx: ExprContext::Load,
range: TextRange::default(),
})),
slice: Box::new(Expr::Tuple(ast::ExprTuple {
elts: type_members
.into_iter()
.map(|type_member| {
Expr::Name(ast::ExprName {
id: type_member,
ctx: ExprContext::Load,
range: TextRange::default(),
})
})
.collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
})),
ctx: ExprContext::Load,
range: TextRange::default(),
})),
ctx: ExprContext::Load,
range: TextRange::default(),
}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a shorthand for any of this?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's bad 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is NOT what I wanted to hear! 🤣 Thanks anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully clear that I mean the abstractions are bad, and not your code :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤣 of course.

...but why not both?

@charliermarsh
Copy link
Member

I think w: builtins.type[int] | builtins.type[str] | builtins.type[complex] should turn into w: builtins.type[int | str | complex].

Is your first question still relevant? It looks like stuff outside of the def is being flagged in the snapshot.

@diceroll123
Copy link
Contributor Author

I think w: builtins.type[int] | builtins.type[str] | builtins.type[complex] should turn into w: builtins.type[int | str | complex].

Is your first question still relevant? It looks like stuff outside of the def is being flagged in the snapshot.

mmm, that answers my question! I'll dig into that. Thanks!

@charliermarsh
Copy link
Member

On second thought, it's probably not worth the complexity. But we should ensure that checker.is_builtin("type") before adding the autofix.

@diceroll123
Copy link
Contributor Author

On second thought, it's probably not worth the complexity. But we should ensure that checker.is_builtin("type") before adding the autofix.

checker
        .semantic()
        .resolve_call_path(subscript.value.as_ref())
        .is_some_and(|call_path| matches!(call_path.as_slice(), ["" | "builtins", "type"]))

is this code starting on line 83 not doing that? or is it not explicit enough?

@charliermarsh charliermarsh added the fixes Related to suggested fixes for violations label Oct 13, 2023
@charliermarsh
Copy link
Member

It's not quite sufficient, because that would also match builtins.type. I just added a guard to check the builtin explicitly prior to performing the fix.

@charliermarsh charliermarsh enabled auto-merge (squash) October 13, 2023 00:46
@charliermarsh charliermarsh merged commit 1e184e6 into astral-sh:main Oct 13, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants