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

[flake8-pytest-style] Add automatic fix for pytest-parametrize-values-wrong-type (PT007) #10461

Merged
merged 6 commits into from
Mar 18, 2024

Conversation

autinerd
Copy link
Contributor

Summary

This adds automatic fixes for the PT007 rule.

I am currently reviewing and adding Ruff rules to Home Assistant. One rule is PT007, which has multiple hundred occurrences in the codebase, but no automatic fix, and this is not fun to do manually, especially because using Regexes are not really possible with this.

My knowledge of the Ruff codebase and Rust in general is not good and this is my first PR here, so I hope it is not too bad.

One thing where I need help is: How can I have the transformed code to be formatted automatically, instead of it being minimized as it does it now?

Test Plan

Using the existing fixtures and updated snapshots.

@charliermarsh charliermarsh self-requested a review March 18, 2024 18:00
@charliermarsh charliermarsh added the fixes Related to suggested fixes for violations label Mar 18, 2024
@charliermarsh
Copy link
Member

Thanks for this!

One thing where I need help is: How can I have the transformed code to be formatted automatically, instead of it being minimized as it does it now?

So, if you want to do this, you can't use generator. The generator is just a basic printer that takes an AST and prints it without formatting. If you want to preserve the existing formatting (including comments), you need to implement a more manual fix (e.g., change the [ to ( and the ] to ), or similar). Check out unnecessary_generator_set for an example of what that might look like.

I'm fine to merge with the generator-based fix, though a locator-based fix tends to preserve more of this kind of trivia. Are you interested in trying it?

@autinerd
Copy link
Contributor Author

Oh yes, I see it now that this approach deletes all comments. Thanks for pointing me to a place to check, I will try if I can do this.

@charliermarsh
Copy link
Member

Yeah it's kind of a tradeoff. The generator is much easier (to program, and to get right), but much lower-fidelity.

Copy link
Contributor

github-actions bot commented Mar 18, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@autinerd
Copy link
Contributor Author

autinerd commented Mar 18, 2024

So, I have it now as a locator-based fix based of unnecessary_generator_set, thanks!

It also checks if a list has only 1 element and transforms ["test"] into ("test",) to make it a valid tuple.

The only thing is that ("test",) will be converted into ["test",], which is valid from the syntax, but of course not the most beautiful.

@autinerd
Copy link
Contributor Author

And I have it now as a unsafe fix, but I think it should be safe to use in this approach.

@autinerd
Copy link
Contributor Author

Oh, ruff-format seems to transform ["test",] unconditionally into

[
    "test",
]

Then maybe I need to remove the comma, because this disrupts the source formatting.

@charliermarsh
Copy link
Member

You could, e.g., only remove the trailing comma if it immediately precedes the closing brace.

@autinerd
Copy link
Contributor Author

autinerd commented Mar 18, 2024

I'm sorry, but can you maybe hint me on how to access the text of the Expr to that I can check on whether it ends with ,)?

Ah, I'm blind, I see it now.

@autinerd
Copy link
Contributor Author

It doesn't look very elegant in my opinion, but it works.

@charliermarsh
Copy link
Member

Thanks! I'll take a look now. I may tweak the locator usage a bit but it's generally what I was suggesting.

@@ -79,5 +79,6 @@ def test_single_list_of_lists(param):

@pytest.mark.parametrize("a", [1, 2])
@pytest.mark.parametrize(("b", "c"), ((3, 4), (5, 6)))
@pytest.mark.parametrize("d", [3,])
Copy link
Member

Choose a reason for hiding this comment

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

Just one bugfix: the PR was adding a trailing comma here, which led to ,,

@charliermarsh charliermarsh changed the title Add automatic fixes for PT007 [flake8-pytest-style] Add automatic fix for pytest-parametrize-values-wrong-type (PT007) Mar 18, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) March 18, 2024 20:22
@charliermarsh charliermarsh merged commit 1a2f9f0 into astral-sh:main Mar 18, 2024
17 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