Workflow for contributors

[milancurcic]

Use this issue to discuss the workflow that we should adopt. This will eventually become part of the Workflow doc

For the time being:

• Use What should be part of stdlib? #1 to broadly discuss what should be part of stdlib;
• Open a new issue for a specific proposal;
• Once the proposal gets reasonably enough discussion and support from the community, open a pull request to add code to the stdlib;
• We can flesh out the specific requirements regarding quality control and similar in this thread.

We can also discuss the preferred git/GitHub workflows for contributing to the reop, such as feature branches, pull requests, etc.

[zbeekman]

CD and conventional commits/automatic semantic versioning

I propose we adopt conventional commits https://www.conventionalcommits.org/en/v1.0.0/ and semantic versioning https://semver.org.

Conventional commits are a means of ensuring compliance with semver and automating the release process and changelog generation.

As such I propose master is where stable code is continuously delivered and released, devel is a protected development branch and all contributions are done via a forking workflow on feature branches (on the contributors fork).

[certik]

The "conventional commits" is an interesting idea, but it seems way too complicated, and for that reason the projects that are listed there as example projects do not actually follow it, e.g.:

• BlazeSoftware/blaze#220
• yargs/yargs#1506

The reason is that most new contributors do not know how to format their commits properly, and thus you can either reject such contributions or it puts more work on project maintainers to rebase / squash commits properly, and squashing is bad because it destroys history and removes attribution if there is more than one author in the PR.

A better approach is to tie the changelog to the PR itself, not the individual commits. We do that in SymPy, here is an example:

• sympy/sympy#17974

And here is documentation about how it works: https://github.com/sympy/sympy/wiki/Writing-Release-Notes. We have a tool that creates the release notes automatically from the description in the PR. That way the project maintainers can easily create proper changelog description for each PR that anybody submits, and there is no need to rewrite commit history, so it scales well with people.

[zbeekman]

I agree that conventional commits can be a little bit awkward. However I've developed CMake modules for some of my projects that detect when being built from a git repository, install (locally) the required npm modules, and then inject a git hook so that git commit really fires up git-cz. After the interactive commit formatting via git-cz the message is opened in your editor for review and modification.

The largest benefit of this is that commit messages end up clearly indicating breaking changes, and what types of changes were made, not to mention the large eco-system of tools for automatically releasing, versioning, creating changelogs, etc.

But this is not my hill to die on, and the reduced complexity and/or burden on contributors might be worth not using conventional commits. Basically, you can trade burden on committers for complexity in ensuring people have git hooks setup properly etc.

[milancurcic]

I took a look at Conventional Commits and am personally not a fan. Seems like a lot of technical bureaucracy for not much reward. I don't think I've written more than a single-line, simple sentence commit message.

Changelogs in PR make sense to me.

Semver.... sure. This becomes meaningful at scale when people start using this in production. We'll probably be in 0.1.x for some time.

I like the master for latest stable and develop in feature branches, e.g. feature/ascii or similar.

[zbeekman]

Sure, that's fine. They have a nice tool that holds your hand as you write the commit message and can be installed as a hook. But I'm happy to use less structured commit messages if people prefer.

[milancurcic]

Let's bring back the discussion on workflow. We're very early on in the project and learning as we go what will work best for this community. Let's keep the discussion to how we work as people and open separate issues for topics such as versioning or any CI-related technicalities.

What do you think about this flow:

1. You have an idea or a proposal. Open an issue to discuss it. This is on the level of "is there interest in having image reader/writer functions in stdlib?"
2. When there seems to be significant interest in the proposal, like 80/20 participants think it's a good/bad idea, move on to discuss the specific API. It's OK to propose the API off the bat if you already have an idea for it.
3. Discuss the API and iterate. When there is ~80/20 approval for the API, move on to implement it and submit a PR. Small PRs are always better than large. It's OK to implement only a few functions of a new module, and continue work on the others in a later PR.
4. When opening a PR, request reviews from one or more people that are most relevant to it. These are likely to be people involved in prior steps of the workflow. Other contributors (not explicitly invited) may provide reviews and suggestions as well. Iterate until all (or most) participants are on the same page. We should never merge before requested reviewers approve.

[certik]

@milancurcic in addition, I would mention in step 3. that new features go into "experimental" modules. In step 4. I would change the wording from "Other contributors (not explicitly invited) may provide reviews and suggestions as well." to "Other contributors (not explicitly invited) are encouraged to provide reviews and suggestions as well." I would change "We should never merge before requested reviewers approve." to "We should not merge if there is a very strong objection from the reviewers or from somebody in the wider community."

Finally, we need to figure out a step "5. Moving from experimental to main". We can get there in few months once we get actual real world usage of the features in experimental.

[zbeekman]

I agree with the proposal above from @milancurcic and suggestions from @certik.

A more logistical question: Do we always want to merge PRs with non-fast-forward merge commits? Do we ever want to be able to fast-forward merge? Or rebase then fast-forward? (no merge commit created)

The potential advantage of this is that it makes git history easier to understand and simpler. The disadvantage is that it can be harder or less obvious to figure out how to revert an entire merge/PR.

In general, I'm against fast-forward merges of PRs, but recently I have been seeing the appeal for using them in simple cases where there are only one or two commits and the changes in question are well contained.

[certik]

I would keep using the default merge commits --- the main advantage that I can see is that when I need to find the associated discussion and review for a given commit in history, one just has to find the merge commit and it has the PR number in it, so one can go to GitHub and read the discussion to understand why it was merged.

[zbeekman]

ah, I see. So you'd advocate against doing a local merge and push as well. In that case I will refrain from doing that too! (And, I didn't think of that, but for this project that is an extremely compelling case to merge through the GH interface given the amount of discussion we can expect on PRs! Good call!)

[certik]

@zbeekman Yes, exactly. I don't think local merge and push is even allowed is it? I thought I disabled it.

Update: I guess it might still be allowed. In GitLab I always disable direct pushes into master (it's actually really easy to make a mistake and push into the master by accident!), but I guess that is not possible in GitHub? The master branch protection is enabled and "Require status checks to pass before merging", so I thought that would disable direct pushes, because then obviously one cannot guarantee that the CI will pass. But I guess it does not do that.

Either way, yes, please let's not push directly to master. Always create a PR, and always get the PR reviewed and all tests pass before merging.