probabl-ai / skore

Skore let's you "Own Your Data Science." It provides a user-friendly interface to track and visualize your modeling results, and perform evaluation of your machine learning models with scikit-learn.
https://probabl.ai
MIT License
11 stars 0 forks source link

ci: Enforce conventional commit format in PR title #398

Closed augustebaum closed 10 hours ago

augustebaum commented 1 week ago

Closes #66

tuscland commented 5 days ago

I love this! Can we mention expected rules in the contributing guide?

augustebaum commented 4 days ago

He can therefore easily override this PR title rule.

This is a good point; I will do some research to see how to deal with this. In the meantime, only people with write access to the repo (i.e. the Probabl product team) can do this when merging a PR, so as long we're minimally disciplined I don't think that's an issue

augustebaum commented 4 days ago

it should be done at the commits level.

You mean check individual commits inside a PR? I don't mind, it just heavily increases dev friction and in the end all the commits are squashed

augustebaum commented 4 days ago

it's premature

This is a pre-requisite for proper release management, e.g. for updating the changelog automatically. It's a simple change, and it will immediately make our main branch cleaner.

thomass-dev commented 4 days ago

He can therefore easily override this PR title rule.

This is a good point; I will do some research to see how to deal with this. In the meantime, only people with write access to the repo (i.e. the Probabl product team) can do this when merging a PR, so as long we're minimally disciplined I don't think that's an issue

Time for joke: if we're disciplined, we don't need such a test, since only the maintainers can merge a PR and therefore edit the merge-commit message and respect the convention :) .

I'm truly convinced that if we want to base an automatic release management on conventional commits, we need to add some strong rules that can't be ignored. To err is human.


I don't know if its possible to run a workflow after squashing but before merging.

tuscland commented 4 days ago

I agree with @thomass-dev. We can start by describing good conventions, exercise them manually and then implement them automatically.

e.g. for updating the changelog automatically

I am not sure. I don't like changelogs based solely on automatically generated statements; they should primarily have an editorial perspective.

augustebaum commented 1 day ago

Time for joke: if we're disciplined, we don't need such a test, since only the maintainers can merge a PR and therefore edit the merge-commit message and respect the convention :) .

Obviously, you need much less discipline to not change a green PR message, than to make sure every PR you review abides by a full convention. This is a net positive.

We can start by describing good conventions, exercise them manually and then implement them automatically.

I don't think this convention will be strictly followed unless there are automatic checks for it; furthermore, I don't want to do a job that this PR could easily achieve.

I don't like changelogs based solely on automatically generated statements.

Point taken; taking the list of commits is a first step, that can then be used to write up more digestible updates.