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

Clarify check argument is needed for github action workflow #3325

Merged
merged 3 commits into from
Oct 15, 2022

Conversation

jlplenio
Copy link
Contributor

Description

I used the Black github action workflow in one of my projects and passed arguments via options. Because I had initially omitted '--check', my workflow did not fail, although Black found files that needed formatting. I would like to suggest this one sentence that makes that even clearer. It would have saved me some debugging time, as I am quite new to GitHub action workflows.

Clarifies that the check argument is required so that the workflow fails when Black finds files that need to be formatted based on the correct exit codes.

I have successfully run the docs build test locally.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

I agree and approve this extra addition. I feel it's too late now, but we should have probably defaulted with --check.

Should we think about a Major version release changing this default in the future? What are the downsides people see other than helping people find unformatted files?

@cooperlees cooperlees added the skip news Pull requests that don't need a changelog entry. label Oct 10, 2022
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Let's just please our prettier formatter on this markdown file please.

@jlplenio
Copy link
Contributor Author

I was unable to reproduce the prettier error on my WSL. He somehow does not care about line length.
I used the suggested edit from the failed pre-commit workflow.

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

I'm okay with adding --check to the argument list unconditionally starting with 23.0.0.

@ichard26 ichard26 added this to the Release 22.11.0 milestone Oct 14, 2022
Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

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

LGTM and a forced --check sounds good @ichard26 👍

@felix-hilden felix-hilden merged commit 575220f into psf:main Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants