-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add --workers CLI parameter (fixes #2513) #2514
Conversation
I think you meant to link issue #2513 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this approach was okayed in the linked issue, I think we can start polishing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel and agree this is the way to go as our main dislike for CLI args is around formatting changes.
I feel we could also use more click features to simplify the code + actually show how many workers the default detection would run in your environment in --help
But I won't block on requiring these changes if others disagree.
src/black/__init__.py
Outdated
type=int, | ||
help="Number of parallel workers [default: os.cpu_count()]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type=int, | |
help="Number of parallel workers [default: os.cpu_count()]", | |
default=os.cpu_count(), | |
type=int, | |
show_default=True, | |
help="Number of parallel workers", |
Maybe we could just show the cpu_count in the help output ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be confusing from one machine to another and on our documentation. Probably not that big of a deal though, and it is convenient to see the value right away 🤔
I added your changes (use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - If no one objects I'll merge later today.
@click.option( | ||
"-W", | ||
"--workers", | ||
type=click.IntRange(min=1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, didn't know this was also a thing 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well, thanks for the updates!
@click.option( | ||
"-W", | ||
"--workers", | ||
type=click.IntRange(min=1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, didn't know this was also a thing 👌
Description
Fixes Issue #2513. This does introduce a new CLI parameter but it fixes a pretty core issue. See the issue for more details.
Checklist - did you ...
--help
is automatically inserted into the docs.