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

feature/black command line args #3

Merged
merged 25 commits into from
Jun 26, 2020

Conversation

CorreyL
Copy link
Collaborator

@CorreyL CorreyL commented May 7, 2020

Hello @akaihola,

My team wishes to add darker as a pre-commit hook to our Python codebases, but we need support for the --line-length (-l) and --skip-string-normalization (-S) command line arguments to black, as outlined here:

https://black.readthedocs.io/en/stable/installation_and_usage.html#command-line-options

To add support for these options, I modified your invocation of black.FileMode here:

5c606cf#diff-c021797c036b1ba8d6aac676393ca67aR95-R103

To parse the dict that is being now being passed from main, and set the arguments to black.FileMode accordingly.

Note that I've set the defaults for the added command line arguments in accordance with the defaults black uses

class black.FileMode(
  # ...
  line_length: int = 88,
  string_normalization: bool = True,
  # ...
)

Do note that the command line argument is --skip-string-normalization, whereas the argument to black.FileMode is string_normalization, hence the inverse of the darker -S command line argument here:

5c606cf#diff-c021797c036b1ba8d6aac676393ca67aR97-R101

This all being said, there may have been a good reason why you opted not to support these additional command line arguments, in which case I will understand if you choose not to merge this pull requests.

Nevertheless, if you deem this a worthy idea of merging into the main repo, please let me know if I can make any additional adjustments such that you'd agree to merge my added code.

Thanks,
Correy

`black` supports the the command line arguments `--line-length` (-l) and
`--skip-string-normalization` (-S), and can be invoked through the
`black.FileMode` class

As such, add arg parse support for `darker` for `--line-length` and
`--skip-string-normalization` to pass into `black.FileMode`

Set the default values of the args to their `black` defaults, ensuring
that they are not required arguments to run `darker`
* Add an additional `dict` argument to `__main__.format_edited_parts`
  and `black_diff.run_black` as the parameter to store arguments
  intended for the `black.FileMode` instantiation used in `run_black`
* Parse the args `line_length` and `skip_string_normalization` from
  `args` in `main`, bundle them in a `dict` and add as a parameter to
  the invocation of `format_edited_parts`, ensuring that the arguments
  reach the instantiation of `black.FileMode` in `run_black`
Ensures users are aware of what command line arguments will be forwarded
to `black`
Copy link
Owner

@akaihola akaihola left a comment

Choose a reason for hiding this comment

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

Thanks @CorreyL, this is a great addition! Please consider my minor suggestions in the review.

README.rst Outdated Show resolved Hide resolved
src/darker/black_diff.py Show resolved Hide resolved
src/darker/command_line.py Outdated Show resolved Hide resolved
src/darker/command_line.py Outdated Show resolved Hide resolved
@akaihola
Copy link
Owner

Also @CorreyL there's #1 which adds support for using a Black configuration file. I wonder if there's something these two PRs should take into account considering each other?

@akaihola
Copy link
Owner

Also please add your information to the CONTRIBUTORS.rst file!

@akaihola
Copy link
Owner

This new feature deserves a mention in the change log (CHANGES.rst). Could you add that as well?

CorreyL and others added 5 commits June 21, 2020 15:52
- Rename `code` to `code-block` to be consistent with the rest of the README file
- Correct the language specifier from `sh` to `shell`, per the specifications of `reStructuredText`

Co-authored-by: Antti Kaihola <13725+akaihola@users.noreply.github.com>
Ensure the PEP8 convention is being followed for closing off triple-double-quote blocks

Co-authored-by: Antti Kaihola <13725+akaihola@users.noreply.github.com>
For the option `--skip-string-normalization`, use the help string that's
used in `black`, for the sake of consistency

Co-authored-by: Antti Kaihola <13725+akaihola@users.noreply.github.com>
For the option `--line-length`, use the help string that's used in
`black`, for the sake of consistency

Co-authored-by: Antti Kaihola <13725+akaihola@users.noreply.github.com>
@CorreyL
Copy link
Collaborator Author

CorreyL commented Jun 21, 2020

This new feature deserves a mention in the change log (CHANGES.rst). Could you add that as well?

I'd be happy to @akaihola! Did you have a version number in mind for this additional feature? I was thinking 0.3.0, following:

MINOR version when you add functionality in a backwards compatible manner

Source: https://semver.org/

But I'm happy to adhere to whatever you'd prefer =)

@CorreyL
Copy link
Collaborator Author

CorreyL commented Jun 21, 2020

Also @CorreyL there's #1 which adds support for using a Black configuration file. I wonder if there's something these two PRs should take into account considering each other?

I'd be happy to integrate the code from #1 into my branch, and make any necessary changes, when it has been approved and merged into master

@CorreyL
Copy link
Collaborator Author

CorreyL commented Jun 21, 2020

Also please add your information to the CONTRIBUTORS.rst file!

Addressed in 74c6c4e

@CorreyL CorreyL requested a review from akaihola June 21, 2020 23:07
@akaihola
Copy link
Owner

@CorreyL

I'd be happy to integrate the code from #1 into my branch, and make any necessary changes, when it has been approved and merged into master

#1 has now been merged. Go ahead!

Did you have a version number in mind for this additional feature? I was thinking 0.3.0

Seems like a good idea. Actually, merging #1 already bumped the version number to 0.3.0.dev (with .dev to be removed when the release is done).

Into `feature/black-command-line-args`

Make `black_args` a temporarily un-used variable for `run_black`, where
subsequent commits will re-instate its usage
The default values are now handled inside of `run_black`, since it uses
the function `read_pyproject_toml` from `black`, which will handle
setting default values if the keys are not defined in a `pyproject.toml`
file
Add logic to `run_black`, dictating that if command line arguments for
`--line-length` and/or `--skip-string-normalization` were specified,
override both the default values and anything specified in a
`pyproject.toml` file, per the `black` docs:

Command-line options have defaults that you can see in --help. A
pyproject.toml can override those defaults. Finally, options provided by
the user on the command line override both.

Source:
https://black.readthedocs.io/en/stable/pyproject_toml.html#lookup-hierarchy
@CorreyL
Copy link
Collaborator Author

CorreyL commented Jun 25, 2020

@akaihola

From PR #1, for the merge conflict that arised in 8b9c0c1, note that I've merged the function signatures such that black_args is still a required param for the relevant functions, and config is still an optional param:

8b9c0c1#diff-1e1c88d1d01cbbcc466495c946c76221R23-R25

8b9c0c1#diff-c021797c036b1ba8d6aac676393ca67aR102

Feel free to let me know if the changes in 0e822e9 or 4422679 could be made more clear.

Per the commit message, the rationale behind the logic in 4422679 can be justified by the black docs:

Command-line options have defaults that you can see in --help. A pyproject.toml can override those defaults. Finally, options provided by the user on the command line override both.

Source: https://black.readthedocs.io/en/stable/pyproject_toml.html#lookup-hierarchy

CorreyL and others added 6 commits June 24, 2020 21:48
No need for a separate `config` parameter in
- `format_edited_parts()`
- `run_black()`
Other options are handled in parts of Black code base which are not
run when we call `format_str()` directly.
Override configuration file options first with command line options,
then translate the result into parameters for `FileMode()`. This
simplifies parameter handling a bit.
CHANGES.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
src/darker/black_diff.py Outdated Show resolved Hide resolved
src/darker/__main__.py Outdated Show resolved Hide resolved
CorreyL and others added 4 commits June 25, 2020 16:13
Follow the standard convention @akaihola follows, only setting the
version number in the same commit that the release is being made

Co-authored-by: Antti Kaihola <13725+akaihola@users.noreply.github.com>
@CorreyL CorreyL requested a review from akaihola June 25, 2020 23:27
Ensures consistency with help text printed from `command_line.py`

Co-authored-by: Antti Kaihola <13725+akaihola@users.noreply.github.com>
@CorreyL CorreyL force-pushed the feature/black-command-line-args branch from feb1dbc to 15cb524 Compare June 25, 2020 23:33
@CorreyL
Copy link
Collaborator Author

CorreyL commented Jun 25, 2020

Note that I mangled my commit message in feb1dbc, hence the force push of 15cb524

Ensures users are aware that the `-c`/`--config` command that is in
`black` is supported by `darker`
@akaihola akaihola merged commit cdd3369 into akaihola:master Jun 26, 2020
@akaihola akaihola added this to the 1.0.0 milestone Jul 11, 2020
@akaihola akaihola added the enhancement New feature or request label Jul 11, 2020
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