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

Fix AST verification and historical comparisons #138

Merged
merged 12 commits into from
Jun 26, 2021

Conversation

akaihola
Copy link
Owner

@akaihola akaihola commented May 16, 2021

This pull request fixes three bugs:

AST verification no longer erroneously fails when using --isort.

This bug was introduced in #94 and is present in version 1.2.3.

Historical comparisons, e.g. comparing two past commits like darker --diff v1.0..v1.1 would still compare against the file on disk in the working tree, not the second commit. This has been fixed.

Darker would write historical reformats on disk, overwriting a newer version in the working tree. Now an exception is raised instead if a historical comparison like darker --revision=commit1..commit2 is used without --diff or --check.

@akaihola akaihola added the bug Something isn't working label May 16, 2021
@akaihola akaihola added this to the 1.2.4 milestone May 16, 2021
@akaihola akaihola requested a review from samoylovfp May 16, 2021 10:48
@akaihola akaihola self-assigned this May 16, 2021
@akaihola akaihola linked an issue May 16, 2021 that may be closed by this pull request
@akaihola
Copy link
Owner Author

akaihola commented May 16, 2021

@samoylovfp, I assigned this to you since we collaborated also on #94 and you may remember the change I made there.

The description isn't actually accurate currently, I'm trying to rethink this through to figure out what's happening exactly.

Edit: Can't figure it out now, just made the description a bit more general. Note that I found another bug which is related to the same code paths, and included the fix for that one in this PR as well.

@akaihola akaihola changed the title Fix AST verification Fix AST verification and historical comparisons May 16, 2021
@akaihola akaihola force-pushed the ast-verify-perfectly-formatted branch 2 times, most recently from 5e245b2 to 5c4a9db Compare May 16, 2021 20:47
@akaihola akaihola force-pushed the ast-verify-perfectly-formatted branch from 5c4a9db to c4ca2af Compare May 17, 2021 18:45
@akaihola akaihola requested a review from ivanov May 17, 2021 19:06
@akaihola
Copy link
Owner Author

@ivanov, I added you as a reviewer just in case you'd be interested – you mentioned this PR in the discussion of #140.

@akaihola
Copy link
Owner Author

@ivanov and/or @samoylovfp, do you think you'll be able to review this?

One option I'd like to try at some point in this project is to have a pair/group video call session where a pull request author walks through their proposed changes. Maybe this PR is a candidate for such?

@akaihola akaihola force-pushed the ast-verify-perfectly-formatted branch from c4ca2af to 975e3ee Compare May 22, 2021 19:32
@sourcery-ai
Copy link

sourcery-ai bot commented May 22, 2021

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 0.95%.

Quality metrics Before After Change
Complexity 5.00 ⭐ 5.09 ⭐ 0.09 👎
Method Length 63.22 🙂 65.04 🙂 1.82 👎
Working memory 12.23 😞 12.67 😞 0.44 👎
Quality 63.59% 🙂 62.64% 🙂 -0.95% 👎
Other metrics Before After Change
Lines 1293 1400 107
Changed files Quality Before Quality After Quality Change
src/darker/main.py 40.40% 😞 35.62% 😞 -4.78% 👎
src/darker/git.py 70.26% 🙂 72.29% 🙂 2.03% 👍
src/darker/utils.py 82.28% ⭐ 82.28% ⭐ 0.00%
src/darker/tests/test_git.py 63.79% 🙂 62.98% 🙂 -0.81% 👎
src/darker/tests/test_main.py 61.16% 🙂 62.56% 🙂 1.40% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
src/darker/main.py format_edited_parts 21 😞 243 ⛔ 17 ⛔ 27.06% 😞 Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
src/darker/main.py main 19 😞 262 ⛔ 16 ⛔ 28.52% 😞 Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
src/darker/tests/test_main.py test_main 0 ⭐ 257 ⛔ 33 ⛔ 39.08% 😞 Try splitting into smaller methods. Extract out complex expressions
src/darker/tests/test_git.py test_git_get_modified_files 4 ⭐ 177 😞 17 ⛔ 46.33% 😞 Try splitting into smaller methods. Extract out complex expressions
src/darker/tests/test_git.py test_git_check_output_lines 2 ⭐ 127 😞 40 ⛔ 47.30% 😞 Try splitting into smaller methods. Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Let us know what you think of it by mentioning @sourcery-ai in a comment.

@akaihola akaihola mentioned this pull request May 25, 2021
@akaihola akaihola mentioned this pull request May 25, 2021
3 tasks
@akaihola
Copy link
Owner Author

I re-reviewed this myself. Merging now since other contributors are busy.

@akaihola akaihola merged commit dadd96e into master Jun 26, 2021
@akaihola akaihola deleted the ast-verify-perfectly-formatted branch June 26, 2021 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

AST verification fails if a modified file is perfectly formatted
1 participant