Workflow for contributors about stdlib HOT 21 CLOSED

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.

from stdlib.

@certik is right that we do this for every feature PR, but @MarDiehl raises an important point: This isn't obvious from the workflow doc. We should require tests in step 4 (implementation).

@MarDiehl Do you mind opening a PR to add a brief statement about tests in the workflow doc?

(of course, this can't answer the question what makes good and sufficient tests)

from stdlib.

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.

from stdlib.

@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.

from stdlib.

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!)

from stdlib.

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.

If you set master a protected branch then you aren't allowed to push directly to master, unless doing so is equivalent to closing the PR. So you can push a merge commit that corresponds to an open PR and closes it. Then depending on the other settings (i.e. allow squash and rebase) you might be able to also do that locally and push.

Anyway, I'll merge through the online web interface going forwards. It has some downsides (like merge commits created that way won't be signed with my GPG signing key, but they might be signed with a different GH key) but that's fine and preserving the default merge comment is worth it.

from stdlib.

(and, just FYI, I would never intentionally push anything to master other than a merge commit to close an open PR, but I will refrain from doing that as well)

from stdlib.

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).

from stdlib.

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.

from stdlib.

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.

from stdlib.

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.

from stdlib.

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.

from stdlib.

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.

from stdlib.

@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.

from stdlib.

I discussed this with @nncarlson and we both think the final 5. step should be to write a "specification". As an example, we think the current open function in src/stdlib_experimental_io.f90 is read for it. The specification is an independent document that describes the API and the functionality, so that anyone can then create an implementation just based on the document. stdlib then provides a reference implementation. And it is this specification document that should be "approved", by the wide community and hopefully also informally by the standards committee. And if it gets "approved", we can then move code from "experimental" to "main".

I created #94, where I tried to write down this workflow based on the above discussion.

from stdlib.

The specification is an independent document that describes the API and the functionality, so that anyone can then create an implementation just based on the document.

Where should this document be saved? Next to the .f90 file? What about the format (.md)? Wht about the name (e.g., stdlib_experimental_io.md for stdlib_experimental.f90)?

from stdlib.

@jvdp1 thanks for staring a discussion about this.

I was hoping the specification would be in some form that we can then automatically generate online html documentation (as well as perhaps a pdf document?). Also I was hoping we could somehow automatically understand the types of the arguments, something like FORD.

Format should be Markdown I think.

So one option is stdlib_experimental_io.md for stdlib_experimental_io.f90. The other option is inline inside stdlib_experimental_io.f90 like FORD.

Why don't we start with stdlib_experimental_io.md to get started. We can discuss how to automate it further as we go.

from stdlib.

I would strongly recommend to accept only functionality that comes with a set of unit tests for all relevant use cases. It might be a little bit pessimistic, but I consider code without test as broken until a unit test convinces me from the opposite

from stdlib.

@MarDiehl that is exactly what we do. Every PR has to have tests for all new functionality before it gets merged.

from stdlib.

@MarDiehl that is exactly what we do. Every PR has to have tests for all new functionality before it gets merged.
👍

from stdlib.

We have an established workflow that we've been mostly following for a while now. I will close this and we can tackle any problems that come up in the future as specific issues.

from stdlib.