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

DRAFT: Apply pyupgrade. #51

Closed
wants to merge 3 commits into from
Closed

Conversation

Carreau
Copy link
Collaborator

@Carreau Carreau commented Aug 13, 2020


(pyupgrade is "a tool (and pre-commit hook) to automatically upgrade syntax for newer versions of the language.")

Just seeing how it looks like, we would of course need to pass options in, so i'm thinking only once there is a config file; likely.

This is likely incorrect as the edited lines could likely be off once Black has been ran as it can rewrap stuff.

It also does use pyupgrade private internal API, and the author has explicitly stated they they were not willing to have public API beyond the CLI.

We should also likely run this before Black; maybe even before isort? Not sure if it might remove imports... But at least it does not pass the AST unchange part.


Late for today, just submitting in case you decide to also write the same thing.

@akaihola
Copy link
Owner

Oh wow, another splendid idea! But I agree with your notes regarding options, config file first, and the AST check challenge.

Also, I'm not sure if you meant this by "pass options in", but I think this should be off by default and activated using a command line / configuration file option.

@akaihola akaihola added the enhancement New feature or request label Aug 13, 2020
@@ -125,14 +125,42 @@ def format_edited_parts(
# 10. A re-formatted Python file which produces an identical AST was
# created successfully - write an updated file or print the diff
# if there were any changes to the original
result_str = pyup(result_str, edited_lines, edited_linenums)
Copy link
Owner

Choose a reason for hiding this comment

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

I agree with your note about pyupgrade changes being better done before Black reformatting, maybe even as the first thing before isort.


pyup_opcodes = diff_and_get_opcodes(edited_lines, content)
pyupgrade_chunks = list(opcodes_to_chunks(pyup_opcodes, edited_lines, content))
pyup_chosen_lines: List[str] = list(choose_lines(pyupgrade_chunks, edited_linenums))
Copy link
Owner

Choose a reason for hiding this comment

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

edited_linenums isn't necessarily correct any more at this point. If Black has e.g. divided a long line on multiple lines, every line number after that gets shifted. This would cause wrong pyupgrade changes to be selected and desired ones skipped. This is why changes by all tools (pyupgrade, isort, Black) need to be done first, and only then the diff be calculated between the user's original version and the automatically modified one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that what I was thinking, or recomputing the diff WRT the revision after each tool.

I think the problem with (all tools) then (compute diff), is you may end up with chunks larger than you wants.

say original

a = set((1, 2))
b    =     c

user edit

- b    =     c
+ b    =     d

(black + pyupgrade) then (compute diff), will change both lines, which you don't want to apply.
Though,
(black, diff) should be applied, then (pyupgrade, diff) should not.

Copy link
Owner

Choose a reason for hiding this comment

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

There's also the additional potential problem of tools which may change the order of lines in code. isort is an example of such a tool, and that's why Darker just includes modifications from it as part of the starting point, and compares Black results to that. I wonder if any of the rules in pyupgrade could potentially also change the order of lines in the source code?

Here's an illustration of what code reordering would cause if such changes would be attempted to apply to only modified lines:

Let's say I have this module committed to the HEAD of my repository:

1  import  os
2  import black
3  import sys

I then manually correct the first line to import os without the extra space:

*1  import os
 2  import black
 3  import sys

Run isort:

isort myfile.py

Get this as a result:

 1  import os
*2  import sys
*3
*4  import black

Git shows the difference:

git diff -U0
git diff -U0|cat
diff --git a/imp.py b/imp.py
index 61a152d..1ed0c5e 100644
--- a/imp.py
+++ b/imp.py
@@ -1,2 +1 @@
-import  os
-import black
+import os
@@ -3,0 +3,2 @@ import sys
+
+import black

Only the first chunk falls on line 1 (the only edited line) and would be applied by a Darker-like tool. The result would be:

1  import os
2  import sys

So I would actually lose my Black import as a result of applying sorted imports partially.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from my look at upgrade that does nto seem to be the case, it really appears to do AST node replacements, so I would not be too worried.

@Carreau
Copy link
Collaborator Author

Carreau commented Aug 13, 2020

Oh wow, another splendid idea! But I agree with your notes regarding options, config file first, and the AST check challenge.

Thanks !

Also, I'm not sure if you meant this by "pass options in", but I think this should be off by default and activated using a command line / configuration file option.

Yes, off by default would be my preference I think. And the you can opt-in on per-repo config files.
I was also expecting the work on linters you did in the other PR to refactor this area so did not push much on the impl.

@akaihola
Copy link
Owner

@Carreau do you still think this patch would fit Darker's purpose? If so, we probably need to try and rebase on master – I can help there if the conflicts are complicated.

@akaihola
Copy link
Owner

@Carreau, I rebased this on master and removed the actual call to the pyup() function since it needs to be thought through anyway. I only now looked more closely to what you hade already implemented, and I believe we can make this work. Let's see if this happens by version 1.5.0 or only in 1.6.0 of Darker.

@akaihola akaihola force-pushed the pyupgrade branch 3 times, most recently from 096c525 to e1fcd1f Compare March 14, 2022 17:52
@akaihola akaihola force-pushed the pyupgrade branch 3 times, most recently from 75685fb to 8d1ceef Compare March 26, 2022 21:42
@akaihola akaihola force-pushed the pyupgrade branch 5 times, most recently from 0a69c95 to b6d3edd Compare April 5, 2022 16:56
@akaihola akaihola force-pushed the pyupgrade branch 3 times, most recently from f330bc8 to 720c44e Compare April 10, 2022 12:36
Repository owner deleted a comment from sourcery-ai bot Apr 24, 2022
@akaihola akaihola added this to the 1.6.0 milestone Apr 24, 2022
@akaihola
Copy link
Owner

akaihola commented Aug 28, 2022

Support for flynt is cooking in #308. The same approach might fit well for pyupgrade. Let's rebase this after that one has been merged.

@akaihola akaihola added this to the 1.7.0 milestone Sep 17, 2022
@akaihola akaihola force-pushed the pyupgrade branch 3 times, most recently from 185047d to fcc55c3 Compare November 15, 2022 06:07
@akaihola akaihola force-pushed the pyupgrade branch 3 times, most recently from fcaf05c to 0d5ede9 Compare January 10, 2023 18:33
@akaihola akaihola force-pushed the pyupgrade branch 3 times, most recently from 5761f7e to bf3656f Compare January 15, 2023 21:17
Carreau and others added 3 commits January 22, 2023 22:43
Just seeing how it looks like, we would of course need to pass options
in, so i'm thinking only once there is a config file; likely.

So far there's just a function to call `pyupgrade` on given source code.
Applying it to modified lines needs to be implemented, probably best by
refactoring `_reformat_single_file()` so it can be used for different
source-modifying tools.

It also does use pyupgrade private internal API, and the author has
explicitly stated they they were not willing to have public API beyond
the CLI.

We should also likely run this _before_ black; maybe even before isort ?
not sure if it might remove imports... But at least it does not pass the
ast unchange part.
@akaihola
Copy link
Owner

akaihola commented Jan 22, 2023

Flynt support in #308 has been merged. We can now resume implementing pyupgrade support.

@akaihola akaihola modified the milestones: 1.7.0, 1.8.0 Feb 11, 2023
@akaihola
Copy link
Owner

Moving to version 1.8.0. There are a plenty of changes for 1.7.0 already, I now prefer to get it out first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

2 participants