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

isort only if imports at the top of the module were edited #238

Merged
merged 6 commits into from
Apr 10, 2022

Conversation

akaihola
Copy link
Owner

@akaihola akaihola commented Nov 4, 2021

Fixes #72:

The problem with applying isort modifications "surgically" is in the fact that isort may (and often does) change the order of lines.

Could we consider all changes by isort as one contiguous diff chunk, and apply it if any line in edited_linenums falls inside that chunk? Is there an edge case where isort only modifies part of the import lines, and a user edit happens to an import outside that part? I can't see a problem there. The important thing is that either all or none of the isort modifications are applied.

The changes are pretty involved. Here's a summary for reviewers:

src/darker/__main__.py:

  • some variable renames for clarity:
    • path_in_repo -> relative_path_in_rev2
    • src -> absolute_path_in_rev2
    • relative_path -> relative_path_in_rev2
  • _isort_and_blacken_single_file() and _isort_and_blacken_single_file() now accept
    • a pre-created EditedLinenumsDiffer object instead of creating its own
    • a list of files to skip when reformatting using Black, instead of just a boolean for the single file

src/darker/git.py:

  • rename get_rev1_path() to get_path_in_repo()

src/darker/import_sorting.py:

  • apply_isort() now
    • accepts a pre-created EditedLinenumsDiffer object instead of creating its own
    • calls into helper functions refactored out of it:
      • isort arguments are created in _build_isort_args()
      • isort is invoked in _call_isort_code()
    • generates a diff for the result of isort
  • _diff_overlaps_with_edits() does the meat of the new functionality: it checks whether the range of user edits touches the range of reformatted imports

src/darker/diff.py:

  • diff_chunks() now combines diff_and_get_opcodes() and opcodes_to_chunks() behind one call

@akaihola akaihola added the enhancement New feature or request label Nov 4, 2021
@akaihola akaihola added this to the 1.4.0 milestone Nov 4, 2021
@akaihola akaihola self-assigned this Nov 4, 2021
@akaihola akaihola modified the milestones: 1.4.0, 1.5.0 Feb 6, 2022
@akaihola akaihola force-pushed the isort-if-imports-edited branch 4 times, most recently from 7cc0ecf to 031b52a Compare February 23, 2022 15:58
@akaihola akaihola force-pushed the isort-if-imports-edited branch 2 times, most recently from 893aca7 to b5b5f61 Compare February 27, 2022 14:40
@akaihola akaihola force-pushed the isort-if-imports-edited branch 2 times, most recently from f43bed5 to 7504757 Compare March 14, 2022 17:16
Repository owner deleted a comment from sourcery-ai bot Mar 14, 2022
@akaihola akaihola force-pushed the isort-if-imports-edited branch 3 times, most recently from f31df3a to 51dc438 Compare March 26, 2022 21:43
@akaihola akaihola force-pushed the isort-if-imports-edited branch 2 times, most recently from 50df263 to c31ed4e Compare March 30, 2022 17:35
@akaihola
Copy link
Owner Author

@yajo, I noticed you gave a thumbs up for this idea in #72. I could include this change in release 1.5.0 – would you be able to do a code review of my pull request here?

@akaihola akaihola force-pushed the isort-if-imports-edited branch 3 times, most recently from 177ff90 to b6d95b2 Compare April 5, 2022 18:58
@akaihola
Copy link
Owner Author

akaihola commented Apr 5, 2022

I hopefully managed to clarify the changes a bit in the description of this PR to make reviewing easier. @roniemartinez would you happen to have a bit of time to do a sanity check on this patch?

@yajo
Copy link

yajo commented Apr 8, 2022

I've got no idea about the code sorry 😕

However I've tested this and it doesn't seem to do any isort at all. I'm not sure if I'm testing well, though. AFAICS only black runs:

Kooha-04-08-2022-07-31-04.mp4

@akaihola
Copy link
Owner Author

akaihola commented Apr 8, 2022

@yajo, thanks for testing this! You do need the -i/--isort option to sort imports as well. It's not enabled by default.

Is there something we could do to improve documentation so this becomes more obvious?

@akaihola
Copy link
Owner Author

akaihola commented Apr 9, 2022

@roniemartinez, if the review feels too messy, I could also separate out e.g. the variable renames, function refactorings and EditedLinenumsDiffer changes into their own PRs which don't change functionality. Would this help?

Copy link
Collaborator

@roniemartinez roniemartinez left a comment

Choose a reason for hiding this comment

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

LGTM

@akaihola akaihola merged commit 571d4fe into master Apr 10, 2022
@akaihola akaihola deleted the isort-if-imports-edited branch April 10, 2022 09:23
@akaihola
Copy link
Owner Author

Thanks @roniemartinez! We're getting close to a 1.5.0 release!

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.

--isort sorts imports even if they were not edited
3 participants