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

feat: Autofix for W1510 #323

Closed
wants to merge 4 commits into from
Closed

feat: Autofix for W1510 #323

wants to merge 4 commits into from

Conversation

dciborow
Copy link
Contributor

@dciborow dciborow commented Apr 20, 2023

@dciborow dciborow changed the title Autofix for W1510 feat: Autofix for W1510 Apr 20, 2023
@karthiknadig karthiknadig self-assigned this Apr 20, 2023
@karthiknadig karthiknadig added the feature-request Request for new features or functionality label Apr 20, 2023
@karthiknadig karthiknadig self-requested a review April 20, 2023 03:06

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

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

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.

@karthiknadig
Copy link
Member

Closing this since we don't have a good solution for : #323 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants