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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to require a specific version to be running #2300

Merged
merged 3 commits into from Jun 3, 2021

Conversation

felix-hilden
Copy link
Collaborator

@felix-hilden felix-hilden commented Jun 2, 2021

Closes #1246: This PR adds a new option (and automatically a toml entry, hooray for existing configuration management 馃帀) to require a specific version of Black to be running.

For example: black --required-version 20.8b -c "format = 'this'"

Execution fails straight away if it doesn't match __version__. Some points of contention:

  • Option placement: higher or lower on the CLI and help
  • Option name: revision was suggested in the issue, I agree that it's the most appropriate, but it isn't perfect. Other possibilities: runtime-version, black-version..? required-version was chosen
  • Option shorthand: I don't think it's needed
  • Error message: particularly printing the configuration location
  • Documentation: new section name, placement, anything else

if code is None:
click.secho(str(report), err=True)
click.echo(str(report), err=True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bit unrelated but noticed that styling isn't used here, so I changed it.

@ichard26 ichard26 self-requested a review June 2, 2021 13:03
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

A couple of wording comments.

Also, what about --release for the version name? After all, it only works with released versions.

Now going to check if other tools (Prettier, gofmt?) also have options like this that we could steal the name of.

docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
src/black/__init__.py Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Collaborator

Rustfmt uses --required-version: https://rust-lang.github.io/rustfmt/?version=v1.4.36&search=#required_version

Prettier, gofmt, and clang-format don't appear to have a similar option.

@felix-hilden
Copy link
Collaborator Author

That or "release", both sound good to me! I think I'll go with "required version" in the absense of other comments, because it's more explicit. Thanks a lot for the wording improvements!

@@ -368,6 +377,17 @@ def main(
config: Optional[str],
) -> None:
"""The uncompromising code formatter."""
if config and verbose:
out(f"Using configuration from {config}.", bold=False, fg="blue")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the previous printing of config file location on fail, I think it's better to leave it to --verbose.

@felix-hilden
Copy link
Collaborator Author

felix-hilden commented Jun 2, 2021

There ya go, keep the comments coming if something else is off!


Uh, I'm not entirely sure why the test is failing. Before going off the deep end: are those random test failures I've read about still a thing?

@JelleZijlstra
Copy link
Collaborator

Sounds good! The test failure indeed looks random.

It occurred to me that this option name could be confused with --target-version, which is a different kind of version. That's probably fine though.

@felix-hilden felix-hilden changed the title Add option to require a specific revision to be running Add option to require a specific version to be running Jun 3, 2021
@JelleZijlstra JelleZijlstra merged commit a2b5ba2 into psf:main Jun 3, 2021
@jvanasco
Copy link

jvanasco commented Jun 3, 2021

Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request - add required black version in pyproject.toml
4 participants