Add workflows and adjust configuration#8
Conversation
|
There was a problem hiding this comment.
For my own curiosity more than anything else - what does this file affect? Does it create a pre-commit hook for commits that are done through the GitHub site, or is it something that's meant to take effect on developer machines?
There was a problem hiding this comment.
It's supposed to take effect on the developer's machine. This prevents you from committing code that hasn't been formatted using Black. Unfortunately, there have been cases where developers do not properly install this precommit hook, and so unformatted text does show up in the repo.
There was a problem hiding this comment.
Interesting - how does one install that hook? I don't see any directions in the readme, and git commands usually don't take yaml files, is there a tool we're supposed to use?
We could also flip a switch now, to make Black formatting mandatory for pull requests to be accepted: line 35
I'm personally not a fan of pre-commit hooks for a couple reasons:
- I think the point where formatting matters is merge time. I personally like being able to commit stuff that doesn't meet any quality bar, as long as it's not on the develop or main branch, because it lets me use git as a simple backup mechanism. Developers can commit something that doesn't even have correct syntax, just to have a checkpoint at the end of the workday or whatever.
- It requires a manual action by every developer on every checkout - they have to know to install the pre-commit hook. In contrast, if we lean on CI checks, it's all automated and nobody has to remember anything.
There was a problem hiding this comment.
The hook "used" to get automatically installed with setup.py. See here: https://github.com/creare-com/ogc/blob/main/setup.py#L66C7-L66C25 and here: https://github.com/creare-com/ogc/blob/main/setup.py#L102
With Python's move to a pyproject.toml file, I'm not sure how this works anymore.
The main advantages of this approach is that the automatic formatting makes the total codebase smaller, and you don't have to look at all the changes to to formatting when you're reviewing a merge request. In theory it's also automatic with no effort on the developer's part... in practice the hook seems to misbehave. Personally, I'm a fan of doing both the CI check and the hook.
There was a problem hiding this comment.
Personally, I'm a fan of doing both the CI check and the hook.
I can totally get behind that! Seems like one of those situations where the redundancy is helpful.
There was a problem hiding this comment.
Setup.py still gets called with pyproject.toml present. However our PostDevelopCommand does not. In order to get the pre commit install you now need to run python setup.py develop directly. I can add a note on this or come up with a workaround.
There was a problem hiding this comment.
I think we provide a setup.sh file in our python project template to install precommit hooks now... that was the best workaround we came up with.
There was a problem hiding this comment.
This looks great!
As additional work in the future, it would be really cool to get SonarQube to ingest the coverage report from pytest. I got it working in our GitLab setup, but I am not sure how we're meant to hook that into the free cloud version of SonarQube.
I'm gonna merge in this pull request for now though - I think that would be work for a future PR!
3 participants
No description provided.