Autofix for none-not-at-end-of-union #15136 (RUF036)#18964
Autofix for none-not-at-end-of-union #15136 (RUF036)#18964jordyjwilliams wants to merge 7 commits intoastral-sh:mainfrom
none-not-at-end-of-union #15136 (RUF036)#18964Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PD002 | 2 | 1 | 1 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+1 -3 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)
apache/superset (+1 -1 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL
+ tests/integration_tests/model_tests.py:491:29: PD002 `inplace=True` should be avoided; it has inconsistent behavior - tests/unit_tests/pandas_postprocessing/test_rename.py:65:9: PD002 `inplace=True` should be avoided; it has inconsistent behavior
langchain-ai/langchain (+0 -2 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview
- libs/core/langchain_core/language_models/llms.py:158:44: PYI016 [*] Duplicate union member `None` - libs/core/langchain_core/language_models/llms.py:194:44: PYI016 [*] Duplicate union member `None`
Changes by rule (2 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PD002 | 2 | 1 | 1 | 0 | 0 |
| PYI016 | 2 | 0 | 2 | 0 | 0 |
|
I realized this has been in my notification inbox for quite a while, so I wanted to say thanks for your work on this! I'm still planning to review it, I just need to set aside a good chunk of time to go through the issue and previous PR reviews as well as the new changes. |
ntBre
added
fixes
|
@ntBre all good. Happy to address any changes or refactor the approach if needed. Was somewhat a combination and extension of closed PRs. Being totally honest I am pretty new to rust and ruff as a repo so I may have missed something here! |
|
@ntBre could you try to take a look at this PR the coming week? |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks for working on this and apologies again for the delay in reviewing. I have a few high-level suggestions/requests here.
This is mostly for my future reference, but I also tried to pull out a list of things to look for based on the previous reviews:
- single diagnostic and fix for the whole union (#15139 (comment))
- additional mixed union tests (#15139 (comment))
- double check ecosystem check doesn't remove violations (#15139 (comment))
- avoid removing duplicate None entries (#15139 (comment))
- avoid flattening nested U types (#15139 (comment))
I think you've already handled the last two of these 👍
There was a problem hiding this comment.
Would you mind pulling in the tests from the previous PR? I think that would make it a little easier for me to compare between the two.
| // Autofix only if there is exactly one None and no nested unions | ||
| let can_fix = none_exprs.len() == 1 && !has_nested_union; |
There was a problem hiding this comment.
Nice, I think this is a great idea to cut the scope.
| // Add None to the end | ||
| expr_texts.push(none_text); | ||
| // Reconstruct the union string | ||
| let fixed = expr_texts.join(" | "); |
There was a problem hiding this comment.
I don't think we can build the union like this, unfortunately. We need to handle the case of a typing.Union and preserve that union style, not always replace it with a PEP-604-style union. Micha's patch here might be helpful, although he said there were still some edge cases we might have to watch out for.
| // Autofix only if there is exactly one None and no nested unions | ||
| let can_fix = none_exprs.len() == 1 && !has_nested_union; | ||
|
|
||
| for none_expr in none_exprs { |
There was a problem hiding this comment.
I also agree with Micha about just emitting a single diagnostic for the whole union instead of one diagnostic per none_expr (#15139 (comment)). It might make sense to spin that off into a small, separate PR. Basically we'd just call checker.report_diagnostic once and I guess pass in the whole union range. And we can do that without any special preview checks because the rule itself is in preview.
That won't make this PR much easier, but at least we can move the fix generation out of the loop.
|
Thanks for your work on this. I'll close this PR due to inactivity but we're happy to take a look at a re-submission that accounts for Brent's feedback |
3 participants
Summary
none-not-at-end-of-union (RUF036)#15136.autofixfornone-not-at-end-of-unionnone-not-at-end-of-union (RUF036)#15139 (comment)none-not-at-end-of-union (RUF036)#15139 (comment)Test Plan
rustandruffcontributions in general.