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

Fix Style Check step #279

Merged
merged 5 commits into from
Feb 8, 2023
Merged

Fix Style Check step #279

merged 5 commits into from
Feb 8, 2023

Conversation

PProfizi
Copy link
Contributor

@PProfizi PProfizi commented Feb 8, 2023

Pre-commit step is currently failing due to new version of isort not being used. Add update command when installing pre-commit.

Pre-commit step is currently failing due to new version of `isort` not being used. Add update command when installing pre-commit.
@PProfizi PProfizi added the CI/CD label Feb 8, 2023
@PProfizi PProfizi self-assigned this Feb 8, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #279 (5f50e08) into master (3f89cfe) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #279   +/-   ##
=======================================
  Coverage   83.64%   83.64%           
=======================================
  Files          25       25           
  Lines        1351     1351           
=======================================
  Hits         1130     1130           
  Misses        221      221           

@PProfizi
Copy link
Contributor Author

PProfizi commented Feb 8, 2023

Hi @RobPasMue I am trying to fix the problem with pre-commit and isort you mentioned. I tried updating the Style Check step in our CI to match what is done in the pyansys action yet it is not enough.
Does this mean I have to pin isort=5.12.0 somewhere? It does not appear anywhere right now and the pip install pre-commit step does not even seem to install isort.

@RobPasMue
Copy link
Member

RobPasMue commented Feb 8, 2023

HI @PProfizi! Let me fix this quickly. Steps to reproduce my upcoming changes are:

pre-commit autoupdate
pre-commit run --all-files

The version has to be changed at the pre-commit-config.yaml file. Your Python version was fine as it was, no worries about that.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@RobPasMue RobPasMue self-requested a review February 8, 2023 08:47
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

This should work now 😄 thanks for pinging me @PProfizi

@PProfizi
Copy link
Contributor Author

PProfizi commented Feb 8, 2023

This should work now 😄 thanks for pinging me @PProfizi

Thank you so much @RobPasMue!

@PProfizi PProfizi merged commit 4f954a6 into master Feb 8, 2023
@PProfizi PProfizi deleted the ci/fix_pre-commit_check branch February 8, 2023 09:09
@PProfizi
Copy link
Contributor Author

PProfizi commented Feb 8, 2023

@RobPasMue one question though: I thought dependabot would deal with the pre-commit.yaml dependencies, yet I guess I haven't plugged this in.

@PProfizi
Copy link
Contributor Author

PProfizi commented Feb 8, 2023

@RobPasMue one question though: I thought dependabot would deal with the pre-commit.yaml dependencies, yet I guess I haven't plugged this in.

Sorry, just checked, and it does not support it yet I think.

@RobPasMue
Copy link
Member

Nope, that's not supported. What we can do is set up pre-commit.ci though. This tool will automatically open pre-commit related update PRs on a weekly basis. And it will also allow you to remove the style-checks step of your workflows since it runs on cached runners always. See this issue for a summary of the benefits: ansys/pyfluent#1253

If you wanted it on your repository let me know and I can help you set it up 😄

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

Successfully merging this pull request may close these issues.

2 participants