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

Workflow fail if black and isort fail #38

Merged
merged 5 commits into from
Apr 20, 2024
Merged

Workflow fail if black and isort fail #38

merged 5 commits into from
Apr 20, 2024

Conversation

UmairK5669
Copy link
Member

Description:
Ensure that the workflow fails when black and isort style guides are not met.

Related Issues:
#36

Tips for the Reviewer:
Push black and isort breaking changes to observe workflow.

Note: I was unsure if black and isort should still run as they did before, kept them as is for now however, that can be removed as needed.

Checklist:
Please be sure to check all boxes honestly. This is to ensure a smooth development process, and to reduce the
likelihood of needing to make additional changes later on.

  • I understand that changes authored by individuals who have not signed a Contributor License Agreement (CLA),
    or whose authorship we cannot verify, may not be accepted.
  • These contribution address a triaged issue.
  • I have performed a self-review of my code and my changes generate no new warnings.
  • I have included unit tests for my code contributions, and all tests are passing.
  • The docstrings are complete, include doctests demonstrating usage, and are properly formatted.
    I have verified that any updates to the project documentation are complete and look okay.
  • I have confirmed all my code contributions are formatted in accordance with the project's Flake8, Black,
    and isort style configurations.
  • No unnecessary changes have been made to the Poetry lock file, but pyproject.toml and the Poetry lock file
    have been updated to reflect any dependency changes.
  • I acknowledge that upon the submission of this pull request, Qoherent will assume the copyright for this
    contribution.

@UmairK5669 UmairK5669 marked this pull request as draft April 19, 2024 20:57
@UmairK5669
Copy link
Member Author

Seems that when you use the use_pyproject: true option in the workflow, it fails as it can't find black as a dependency in pyproject.toml. Check out the workflow summary of this commit to see more.

Without this flag, it seems our configuration outlined in pyproject.toml is still used (ignore files, max-line length etc) and runs without issues. Check out this workflow summary and expand the "Check Black formatting", which shows Black checking for formatting issues with our configuration. Screenshot for reference provided below:

Screenshot 2024-04-19 at 5 33 59 PM

@UmairK5669 UmairK5669 marked this pull request as ready for review April 19, 2024 21:34
@UmairK5669 UmairK5669 requested a review from mrl280 April 19, 2024 21:34
Copy link
Collaborator

@mrl280 mrl280 left a comment

Choose a reason for hiding this comment

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

Great progress! Perhaps once we remove the "Run Black" and "Run Isort" pieces, we can also remove some of the installs? Not sure if they are required for the "check formatting" sections?

@@ -33,9 +33,19 @@ jobs:
- name: Run Black
run: black ./ --config pyproject.toml
Copy link
Collaborator

@mrl280 mrl280 Apr 19, 2024

Choose a reason for hiding this comment

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

I don't think the "Run Black" section is wanted. The concern is if we format the code before checking it, then the workflow will always pass.

options: "--check --verbose"
src: "./"
# use_pyproject: true, should be used but causing issues so commenting for now

- name: Run Isort
run: isort .
Copy link
Collaborator

@mrl280 mrl280 Apr 19, 2024

Choose a reason for hiding this comment

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

Same comment as above, I don't think we want to keep the "Run Isort" section any more.

with:
options: "--check --verbose"
src: "./"
# use_pyproject: true, should be used but causing issues so commenting for now
Copy link
Collaborator

@mrl280 mrl280 Apr 19, 2024

Choose a reason for hiding this comment

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

More info in my comment below, but I don't think we actually want to use the use_pyproject option.

@mrl280
Copy link
Collaborator

mrl280 commented Apr 19, 2024

Seems that when you use the use_pyproject: true option in the workflow, it fails as it can't find black as a dependency in pyproject.toml. Check out the workflow summary of this commit to see more.

Without this flag, it seems our configuration outlined in pyproject.toml is still used (ignore files, max-line length etc) and runs without issues. Check out this workflow summary and expand the "Check Black formatting", which shows Black checking for formatting issues with our configuration. Screenshot for reference provided below:

It looks like the use_pyproject option might be about the version of Black to use, rather than the Black configuration to use.

It looks like the version of Black the action will use can be read from the pyproject.toml file, and if we set use_pyproject: true, it will look into the tool.black.required-version field in the pyproject.toml file, which we don't have.

As it is now, do we know whether the workflow is using the Black configuration in pyproject.toml? With any luck it might be using it by default?

@UmairK5669
Copy link
Member Author

UmairK5669 commented Apr 20, 2024

As it is now, do we know whether the workflow is using the Black configuration in pyproject.toml? With any luck it might be using it by default?

Haha, I think we got lucky as black's default config is set to 88 per line max as per their docs, and as we can see from the workflow summary the max line length being used is 119 characters. Check it out yourself here (expand the 'Check Black formatting' section.

Screenshot 2024-04-19 at 9 27 33 PM

Also, I removed the force format using black and isort, so the workflow will fail if the code doesn't meet those standards just like with flake8. The pip install black and pip install isort were also removed.

I believe this should now be good to go.

@UmairK5669 UmairK5669 requested a review from mrl280 April 20, 2024 01:30
Copy link
Collaborator

@mrl280 mrl280 left a comment

Choose a reason for hiding this comment

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

Looks good✔️

@UmairK5669 UmairK5669 merged commit 3e261bb into main Apr 20, 2024
2 of 3 checks passed
@UmairK5669 UmairK5669 deleted the format-workflow branch April 20, 2024 04:27
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.

2 participants