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

[pandas-vet] autofixes for PD003, PD004, PD008, PD009 and PD011 #2741

Closed
wants to merge 1 commit into from

Conversation

sbrugman
Copy link
Contributor

Autofixes for PD003, PD004, PD008, PD009 and PD011

let replacement = match *diagnostic.kind.rule() {
Rule::UseOfDotAt => Some("loc"),
Rule::UseOfDotIat => Some("iloc"),
Rule::UseOfDotValues => Some("to_numpy()"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I consider this fix dangerous as there is no check if .values is actually a pandas object.

Copy link
Contributor Author

@sbrugman sbrugman Feb 10, 2023

Choose a reason for hiding this comment

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

The fix is only applied when the use-of-dot-values is detected. (If it's not, then the rule logic should be updated, not that of the fix)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, and use-of-dot-values detects every .values access instead of only pandas related ones: #2229

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thats partially limitation of not having type information. We could try to make the detection more conservative, but if the user enables it consciously, and the detection is correct, then the fix is in principle correct.. I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

My concern here is just that we still have known false-positives for this rule, and so we'll be knowingly breaking code in certain places by applying these autofixes. I'm tempted to say that these should be off-by-default, but enableable by users, which we haven't done before but I'd like to start doing in some cases.

Copy link
Contributor Author

@sbrugman sbrugman Feb 12, 2023

Choose a reason for hiding this comment

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

This one should benefit from the autofix safety levels (#1992). Is there anything we could do in the meantime to merge this, like make the tests more conservative? Implemented this one for autofixing https://github.com/ing-bank/popmon

@sbrugman sbrugman marked this pull request as ready for review February 10, 2023 21:41
@sbrugman sbrugman changed the title autofixes for pandas-vet [pandas-vet] autofixes for PD003, PD004, PD008, PD009 and PD011 Feb 11, 2023
@sbrugman
Copy link
Contributor Author

This PR probably needs autofix aggressiveness levels (#1997)

@sbrugman sbrugman closed this Jul 11, 2023
@zanieb
Copy link
Member

zanieb commented Jul 12, 2023

@sbrugman fwiw those levels are implemented now #4181 although they are not yet respected when running Ruff :)

@sbrugman
Copy link
Contributor Author

Ok, sounds good! Anyone is free to pick up this where it was left off

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.

4 participants