Apply black formatting to the codebase and add enforced status checks on PRs before merging #386
Conversation
CI changes, precommit hook
|
Do you perhaps want to add coverage reporting to the CI/CD? It's fairly straightforward with Pytest and you could publish to a platform like coveralls. This would add the code coverage stats to each PR, and you can see if a new PR increased or decreased coverage, and the lines hit/missed by the tests. Like here for example (C++). You can also set targets and block the merge until a certain coverage target it hit (or when there is a coverage decrease compared to |
|
@namannimmo10 would you be interested to set the coverage up with a badge as @team-gpu suggests? Now we've got the tests running on iris, we can do that. Will be useful to maintain the coverage levels... Thanks! |
|
Sure, will add coverage stats. |
See https://github.com/actions/setup-python/releases/tag/v5.0.0 and https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/ for more details on the node16 warning that this fixes.
See https://black.readthedocs.io/en/stable/integrations/github_actions.html for more details on how it's advised to run `black` via a github action.
yousefmoazzam
changed the title
|
Question for other people's opinion relating to status checks: as mentioned in #273, what do people think about requiring an approved review before merging? |
My opinion: For maintainers, it doesn't matter as they can usually override that setting and merge anyway (unless you want to disable that). For external contributors, they can't merge anyway as they don't have write permission to the main repo. So it only really makes a difference if you have people with write access to this repo, who are not maintainers. Then you can use this to avoid them to accidentally merging something without a review from a maintainer. |
That's a useful nuance to be aware of, thanks! For our case, I think we're unlikely to have a contributor with write access to the repo but isn't a maintainer, so on those grounds, it doesn't make much sense to force an approved review, I'd agree with that. I think a lot of the reason why I'd like the forced approved review is not so much for technical reasons, but more for work culture reasons. I like the idea of encouraging devs to look at the code changes and do code reviews, and not relying solely on the status checks that are in some sense summarising the work that's been done through either a tick or a cross. I'm certainly guilty of this at times too, to not bother reading the code changes and to only look at the status checks, but I'd also say it's more fruitful (and frankly, more fun), to take a look! 🙂 But on the other hand, I do understand that people can be busy and not have time to do a code review, even 10/15 mins for a quick look through can be hard to get done - in which case, forcing an approved review is just forcing a few extra clicks in the github GUI. Maybe that particular mechanism for checking PR readiness should be considered at a later date, and to leave it out of this PR for now. |
Co-authored-by: Yousef Moazzam <[email protected]>
|
I'd say that it is not necessary really, I agree with the @team-gpu comment above. There is unfortunately not enough time to review every single PR and there are not a lot of reviewers as well! |
|
I'll merge it now with the FBP memoryhook failing tests that will be fixed later. And I believe Diad and 360 pipelines will pass when we update httomolibgpu and recent dezinger fixes that are probably not in the 2.0 version that gets installed. |
Sure, that's fine, the IRIS tests action can be left failing for now. However, the black linting action was also failing at the time of merge, it seems like some files were missed maybe? |
|
my black was a bit outdated apparently when I tried to fix it. I've fixed it in |
|
Ok fair enough. I didn't see another PR that fixed it; did I miss it, or are we still allowing pushing to |
Fixes #240
Fixes #272
Main changes:
Checklist