Garden-AI / garden

https://garden-ai.readthedocs.io
MIT License
20 stars 4 forks source link

add auto-formatter (`black`, `yapf`, etc) to GH actions #33

Closed OwenPriceSkelly closed 1 year ago

OwenPriceSkelly commented 1 year ago

as a collaborator on the garden project and a natural pedant, I dislike reading or writing code which is poorly-formatted, and the worst kind of formatting is the inconsistent kind. To ease the cognitive (and emotional) burden of reading or writing poorly-formatted code, we should add a GH action to format the code automatically before the repository is tainted

I currently use black locally, but would be happy to switch to yapf or something else more configurable to accommodate any strongly-held opinions.

Acceptance Criteria

Given proposed changes in a PR, when running GH actions (sometime before the flake8 check), the changes should be formatted by an auto-formatter such as black to be stylistically consistent with the rest of the codebase

steve-barnard commented 1 year ago

@OwenPriceSkelly have you all decided on which format to use (looked like it might have been black)? Also would having a pre-commit hook for formatting be an option? I have seen the pre-commit formatting work well, as this led to fewer issues when formatting conflicts occurred if someone pushed and forgot to pull back in the linted/formatted code.

Actions: https://black.readthedocs.io/en/stable/integrations/github_actions.html

Pre-Commit: https://black.readthedocs.io/en/stable/integrations/source_version_control.html

blaiszik commented 1 year ago

Let's discuss this on Tuesday at the Gathertown meeting. I've noticed, esp in some other repos that our commits are really large when only a few lines are actually changed. We should just pick one as a team and go with it :)

OwenPriceSkelly commented 1 year ago

agreed! especially like the idea of having it as a pre-commit hook. Super open to alternatives, but at this point I think it's probably the path of least resistance to just use black if that's tolerable to everyone

steve-barnard commented 1 year ago

Sounds good! If there is any way to share any current configs for the formatting that would be amazing; I know that when using flake8 and black there can be conflicting information on things like line length etc. Would love to know if there are any current examples of parameters you plan on using such as line length set to 79 or black's 88 or if we are ignoring any other formatting rules;

https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length

Was curious if the line length issues would cause any flake8 errors and cause the build to not complete.

I'm sure this is not a massive priority but would likely allow us to get something set up in the actions workflow as well as document the standards for any future contributors.

OwenPriceSkelly commented 1 year ago

That's a good idea - we might want to document this in a CONTRIBUTING.md or something, but in the meantime / for posterity:

(closing this because I forgot to when I merged #72)