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

Add --check option #278

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Add --check option #278

merged 1 commit into from
Jan 26, 2024

Conversation

JoaquimEsteves
Copy link
Contributor

@JoaquimEsteves JoaquimEsteves commented Aug 23, 2023

✨ New argument: --check

As the name indicates this will merely check if the code would have
been re-written.

A further change is that the return value of blacken-docs is now the
largest error for each file. Previously if file 2 failed with an error
but file 1 would be re-written then the exit code would have been 1 as
opposed to the more severe 2.

Closes #277

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Please add tests and a changelog note.

README.rst Outdated
@@ -118,6 +118,7 @@ It also has the below extra options:

* ``-E`` / ``--skip-errors`` - Don’t exit non-zero for errors from Black (normally syntax errors).
* ``--rst-literal-blocks`` - Also format literal blocks in reStructuredText files (more below).
* ``--check`` - [Similar behaviour to Black's check flag.](https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#check)
Copy link
Owner

Choose a reason for hiding this comment

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

Please write a description of the option rather than just link and say "it's like that". The link is still useful though, but don’t link a whole sentence, just a couple words.

Also alphabetize by moving to line 119.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in f01c5ab

Do tell me if you're unhappy with the phrasing.

return 0
if check_only:
print(f"Error: Would have been re-written - {filename}")
Copy link
Owner

Choose a reason for hiding this comment

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

Let’s match the formatting of the existing message.

Suggested change
print(f"Error: Would have been re-written - {filename}")
print(f"{filename}: Would rewrite.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in: f01c5ab

I did use a different error message:

print(f"{filename}: Requires a rewrite.")

I feel like it reads better; but if you're unhappy I can always just do the one you suggest.

@adamchainz adamchainz changed the title ✨ New argument: --check Add --check option Sep 5, 2023
@JoaquimEsteves
Copy link
Contributor Author

Sorry for the delay; was on holiday.

I also added the tests and the changelog.

From context I think that the changelog requires a version bump; I did so in the setup.cfg but do warn me if I've done it wrong.

@JoaquimEsteves
Copy link
Contributor Author

JoaquimEsteves commented Oct 9, 2023

Dear @adamchainz,

Anything else you'd like to see before merging?

Sorry to be a bother; but since it's been a while so I decided to ping

Copy link

@pfouque pfouque left a comment

Choose a reason for hiding this comment

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

Anything else you'd like to see before merging?

You should rollback the version number bump and remove the release date and version from the changelog (and also bring back the bitwise operator if the usage of max is not required)
I can't commit, but it will be easier to merge once it's done.
Thanks

src/blacken_docs/__init__.py Outdated Show resolved Hide resolved
setup.cfg Outdated
@@ -1,6 +1,6 @@
[metadata]
name = blacken_docs
version = 1.16.0
version = 1.17.0
Copy link

Choose a reason for hiding this comment

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

This change is usually decided by the release manager depending on the changelog at release time: see example

CHANGELOG.rst Outdated
Comment on lines 5 to 6
1.17.0 (2023-09-11)
-------------------
Copy link

Choose a reason for hiding this comment

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

It seems this line is added at release time usually (visible on this release PR)

Suggested change
1.17.0 (2023-09-11)
-------------------

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, I just reviewed and fixed up your PR. Looking good now, going to merge. Thanks for your contribution!

@adamchainz adamchainz enabled auto-merge (squash) January 26, 2024 23:08
@adamchainz adamchainz merged commit 67a0f76 into adamchainz:main Jan 26, 2024
7 checks passed
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.

Add support for a --check argument
3 participants