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.

Sign up for GitHub

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

feat: Autofix for W1510 #323

Closed
wants to merge 4 commits into from
Closed

feat: Autofix for W1510 #323

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a5b437
Update lsp_server.py
dciborow Apr 20, 2023
68e4f5a
Update test_code_actions.py
dciborow Apr 20, 2023
45d5abc
Update test_code_actions.py
dciborow Apr 22, 2023
13fbbf5
Update src/test/python_tests/test_code_actions.py
dciborow Apr 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions bundled/tool/lsp_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,12 @@ def organize_imports(
"repl": r"for \1, \2 in \3.items():",
}
],
"W1510:subprocess-run-check": [
{
"pattern": r"(?<=subprocess\.run\()([^,]*)(?=\))",
"repl": r"\1, check=False",
}
],
}


Expand Down
31 changes: 31 additions & 0 deletions src/test/python_tests/test_code_actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,37 @@ class Banana(object):
print(f"{city} has population {population}.")
""",
"""for city, population in data.items():
""",
),
(
"W1510:subprocess-run-check",
"""
import subprocess

proc = subprocess.run(
["ls"],
cwd=".",
stdout=subprocess.PIPE,
stderr=subprocess.PIPE
)
""",
"""proc = subprocess.run(
["ls"],
cwd=".",
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
check=False
)
dciborow marked this conversation as resolved.
Show resolved Hide resolved
""",
),
(
"W1510:subprocess-run-check",
"""
import subprocess

proc = subprocess.run(["ls"])
""",
"""proc = subprocess.run(["ls"], check=False)
Copy link
Member

Choose a reason for hiding this comment

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

What happens for multiline?

proc = subprocess.run(
    ["ls"],
    cwd=".",
    stdout=subprocess.PIPE,
    stderr=subprocess.PIPE,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.
Might take me a little bit to figure this one out Not sure I have done a multiline fix before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karthiknadig , man am i stuck on this.... looks like this won't fit into the regex pattern easily.

I feel like there should be a simpler way for us to add a new parameter to a function call like subprocess.run, any examples of doing something like this anywhere else that we can go off of?

Copy link
Member

@karthiknadig karthiknadig May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@brettcannon Any recommendation on how we can do this?

Other than actually using a parser to find the right spot, I don't see a way to do this correctly. I honestly think that these change recommendations could be generated from pylint itself. Then all we would have to do is surface them via code actions.

/cc @DanielNoord

Copy link

Choose a reason for hiding this comment

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

We don't have a way to autofix code directly in pylint right now but we have an open issue for this: pylint-dev/pylint#7438. The fix is certainly going to be easier in the context of pylint where we sometime already display the suggestion in the message... vs by regex here :) We also have a lot on our plate so any help would be greatly appreciated and reviewed promptly.

Copy link
Member

Choose a reason for hiding this comment

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

For a keyword argument, you could find the closing ) and then prepend to it, but that's assuming you aren't dealing with any nested parentheses. Otherwise you're wading into using the ast module to parse the code, update the AST, and then regenerate the string representation (which even using ast.unparse() doesn't not guarantee an exact result).

Basically you're playing with 🔥 on this one.

""",
),
],
Expand Down