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

[pylint] Add add_argument utility and autofix for PLW1514 #8928

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

qdegraaf
Copy link
Contributor

@qdegraaf qdegraaf commented Nov 30, 2023

Summary

  • Adds add_argument similar to existing remove_argument utility to safely add arguments to functions.
  • Adds autofix for PLW1514 as per specs requested in Feature request: PLW1514 autofix #8883 as a test

Test Plan

Checks on existing fixtures as well as additional test and fixture for Python 3.9 and lower fix

Issue Link

Closes: #8883

@qdegraaf qdegraaf changed the title Add autofix for PLW1514 [pylint] Add autofix for PLW1514 Nov 30, 2023
Copy link
Contributor

github-actions bot commented Nov 30, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.


if checker.settings.target_version >= PythonVersion::Py310 {
diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
", encoding=\"locale\"".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Would this introduce a syntax error if the call contains no arguments? Are you interested in writing a more robust add_argument utility, similar to the remove_argument that exists today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would. open() and similar calls all have a required first argument AFAIK but it's still a pretty dirty and unsafe fix.

I'd be interested in adding that utility for sure! Will set this to draft and use the autofix as a test case for the utility if there is no urgency behind it.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks @qdegraaf!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented a coarse first version which only handles keyword arguments, but does check for trailing commas and work with empty calls. It explicitly forbids positional arguments but could still maybe break when adding a first argument to a class instantiation.

I could either use the lexer to check for parentheses and panic if they aren't there and leave the implementation of that to a future PR or implement it in this one. What's your preference? If the latter is preferred, if you happen to know of a rule where something like that happens on which I can test the util in absence of unit test infrastructure let me know.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to leave this as function-only for now, so we can assume the parentheses always exist.

@qdegraaf qdegraaf changed the title [pylint] Add autofix for PLW1514 [pylint] Add add_argument utility and autofix for PLW1514 Nov 30, 2023
@qdegraaf qdegraaf marked this pull request as draft November 30, 2023 19:54
@qdegraaf qdegraaf marked this pull request as ready for review December 1, 2023 17:07
@charliermarsh charliermarsh self-requested a review December 1, 2023 17:19
@charliermarsh charliermarsh added the fixes Related to suggested fixes for violations label Dec 1, 2023
@charliermarsh charliermarsh enabled auto-merge (squash) December 1, 2023 18:12
@charliermarsh charliermarsh merged commit 64c2535 into astral-sh:main Dec 1, 2023
16 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.

Feature request: PLW1514 autofix
2 participants