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

Implement code style via pre-commit #264

Merged
merged 10 commits into from
Apr 7, 2022
Merged

Implement code style via pre-commit #264

merged 10 commits into from
Apr 7, 2022

Conversation

jorgepiloto
Copy link
Member

Resolves #261.

@jorgepiloto
Copy link
Member Author

The pylintrc file is not being used because pylint is not installed and neither executed in the CI. I think we can get rid of this file in favor of the other tools.

@dnwillia-work
Copy link
Collaborator

dnwillia-work commented Apr 5, 2022

The pylintrc file is not being used because pylint is not installed and neither executed in the CI. I think we can get rid of this file in favor of the other tools.

Some of us were using it locally. I guess it means installing these other tools locally instead.

You will have to fix up the ci.yml pipeline as well, since it is depending on flake8 right, and run precommit now?

@dnwillia-work dnwillia-work self-requested a review April 5, 2022 10:39
@jorgepiloto
Copy link
Member Author

From private discussion with @seanpearsonuk, he pointed the usage of "pylint" locally, as @dnwillia-work explained in previous comment too.

Knowing this, my suggestion is to keep the usage of "pylint" and move its configuration from "pylintrc -> .pylintrc", see official pylint docs.

@jorgepiloto
Copy link
Member Author

I will also modify the CI actions to work with pre-commit, @dnwillia-work.

Copy link
Collaborator

@dnwillia-work dnwillia-work left a comment

Choose a reason for hiding this comment

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

Jorge, it looks ok though I wonder if it's worth reformatting all the generated code:

  • everything in api.fluent.v0
  • interface defined by fluent.core.meshing.tui.
  • interface defined by fluent.core.solver.tui.
  • interface defined by fluent.core.solver.settings

The first one is generated using grpcio tools, the latter three are generated using codegen/tuigen.py and codegen/settingsgen.py accordingly. The codegen tools should probably be updated to use correct formatting but if that's not easy then I guess we could rely on the code formatter to deal with it.... Maybe you can call the code formatter explicitly after the fact in the codegen modules?

@jorgepiloto
Copy link
Member Author

Thanks for pointing this out, @dnwillia-work. I agree with excluding automated code from the formatting.

It is true that we could execute the formatting tools in the last step of the codegen/*gen.py files, but this would introduce another source of maintenance instead of having all format config under the pre-commit-config.yaml.

@jorgepiloto
Copy link
Member Author

This issue psf/black#438 delayed me a bit when excluding files in .pre-commit-config.yaml. Same applies for flake8. I finally managed to solve it.

@seanpearsonuk seanpearsonuk requested a review from mkundu1 April 6, 2022 08:53
@jorgepiloto jorgepiloto force-pushed the feat/code_style branch 2 times, most recently from b44372d to 7d9580f Compare April 6, 2022 09:39
@jorgepiloto
Copy link
Member Author

Already setup black, isort and flake8. Still working on the codespell and pydocstyle tools for excluding desired files... ⚙️

@jorgepiloto jorgepiloto force-pushed the feat/code_style branch 2 times, most recently from 904672f to 30aaefe Compare April 6, 2022 10:21
@jorgepiloto
Copy link
Member Author

Only task left before labeling this as "Ready for review" is to manually apply code style for flake8, codespell and pydocstyle. This last is going to be most tedious one. I guess we could ignore some of the most common error codes.

@jorgepiloto jorgepiloto marked this pull request as ready for review April 6, 2022 14:19
@jorgepiloto
Copy link
Member Author

Even though I setup [pydocstyle], I finally decided to disable it by commenting its lines in the pre-commit-config.yaml. The reason is due to the large amount of "errors" about required extra lines, missing docstrings... We can address this in a separate issue and merge this PR.

What do you think, @seanpearsonuk @dnwillia-work @mkundu1?

@jorgepiloto jorgepiloto merged commit 50d13fc into main Apr 7, 2022
@jorgepiloto jorgepiloto deleted the feat/code_style branch April 7, 2022 06:14
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.

Enhance code style checking
4 participants