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

--check option, mirroring black --check #36

Merged
merged 4 commits into from
Aug 7, 2020
Merged

--check option, mirroring black --check #36

merged 4 commits into from
Aug 7, 2020

Conversation

akaihola
Copy link
Owner

@akaihola akaihola commented Aug 4, 2020

--check skips writing changes to files, and returns a return value of 1 from the darker process in case any modified lines in any of the given files are not already properly formatted.

The return value will allow using darker with more different tools. It for example opens the possibility for a pytest-darker plugin.

@akaihola akaihola added the enhancement New feature or request label Aug 4, 2020
@akaihola akaihola added this to the 1.1.0 milestone Aug 4, 2020
@akaihola akaihola mentioned this pull request Aug 4, 2020
@akaihola
Copy link
Owner Author

akaihola commented Aug 6, 2020

I rebased on master and resolved conflicts.

Copy link
Collaborator

@samoylovfp samoylovfp left a comment

Choose a reason for hiding this comment

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

Looks great!

A couple of aesthetic nitpicks of questionable usefulness:

  • the call format_edited_parts(but_dont_really_edit_the_files=True) is a little wonky IMO.
  • the fact that the parameter print_diff implies check_only is a little weird to me as well.

I would refactor the code so there is a function
that returns the proposed changes
and another function that actually applies those,
so the high-level control remains high in the call stack and the "do_the_actual_change" flag does not have to travel far in the function calls.

`format_edited_parts()` now yields the proposed changes for each file,
and two separate functions display diffs or modify files.  This way
high-level control remains high in the call stack and the
`--diff`/`--check` flags don't have to travel far in the function
calls.

Thanks for review @samoylovfp!
@akaihola
Copy link
Owner Author

akaihola commented Aug 6, 2020

@samoylovfp, both --diff and --check do imply that changes aren't written into files. The reason for this is simply that it's how Black works. This ensures that Darker can more easily act as a direct replacement for Black when integrating with other tools.

@akaihola
Copy link
Owner Author

akaihola commented Aug 6, 2020

I would refactor the code so there is a function
that returns the proposed changes
and another function that actually applies those

I did that, could you make a another quick check on the code? Quite some test changes were required as a result.

@akaihola akaihola self-assigned this Aug 6, 2020
@samoylovfp
Copy link
Collaborator

@akaihola I think it looks excellent!

@akaihola akaihola merged commit 80ff94d into master Aug 7, 2020
@akaihola akaihola deleted the check-option branch August 7, 2020 19:03
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
None yet
Development

Successfully merging this pull request may close these issues.

2 participants