docs: Docathon 📚

[gmertes]

---

📚 Documentation preview 📚: https://anemoi--22.org.readthedocs.build/en/22/

[FussyDuck]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ gmertes
✅ b8raoult
✅ HCookie
✅ VeraChristina
❌ corentincarton
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[floriankrb]

Great work. Reviewing in readthedocs, I noticed a few things:

_ Should we tell contributors to avoid rewriting history in their PR branches, or not ?

_ in https://anemoi--22.org.readthedocs.build/en/22/contributing/code_style.html
add a link to the .precommit.yaml file ?
"Public API before private implementations." this seems strange. I would do the reverse.

_ is this duplicated doc for "testing" ?
https://anemoi--22.org.readthedocs.build/en/22/contributing/guidelines.html#testing
https://anemoi--22.org.readthedocs.build/en/22/contributing/testing.html

_ "longtests" is not an English word in "@pytest.mark.longtests". I would rather have @pytest.mark.large and/or @pytest.mark.@long

_ in "Configuration Handling", it seems that the "#." syntax is buggy.

[gmertes]

Should we tell contributors to avoid rewriting history in their PR branches, or not ?

Since it will get squashed anyway, does that matter? (just curious)

[VeraChristina]

Thanks for the feedback!

_ is this duplicated doc for "testing" ? https://anemoi--22.org.readthedocs.build/en/22/contributing/guidelines.html#testing https://anemoi--22.org.readthedocs.build/en/22/contributing/testing.html

The "Testing Guidelines" are quite long, so we decided to separate them out and only have a brief section in contributing guidelines with a reference. Since this seems to not have been clear I have shortened that section (also to avoid redundancies), and made the reference to guidelines more explicit -- hope it is more clear now.

_ "longtests" is not an English word in "@pytest.mark.longtests". I would rather have @pytest.mark.large and/or @pytest.mark.@long

I would prefer to not address this in this PR since it requires a change in the testing code. Can you raise this issue in anemoi-core?

_ in "Configuration Handling", it seems that the "#." syntax is buggy.

Good spot! Fixed.

[floriankrb]

Should we tell contributors to avoid rewriting history in their PR branches, or not ?

Since it will get squashed anyway, does that matter? (just curious)

When developing on your own, I like to commit very often on my local clone, so I don't lose my work and I can stash when things get bad. Usually, I don't want to expose all these drafts to the public view, so, I git rebase -i to play around with my commits until I have something I actually want to share, and only then I push.
_ When I am the only one editing the PR code, I feel free to rewrite history in my PR branch and push more often.
_ But not when I am working with others and want to share the current state of the branch. In this case, I do not rewrite history because this will cause issues when they pull.

In a "best practice guideline", we could encourage one behaviour or another. But we also could say nothing about it, as you wrote, this does not matter for the final state of the code in the package : once it has been merged, all these commits are hidden.

[aaron-hopkinson]

A definite improvement and a good starting point to build on in the future. Thanks!

(Added a few comments - feel free to accept/reject these as you see fit)

[aaron-hopkinson]

The line below is for a generic package - should this also be generic (rather than core)? (Maybe we could tie in with the variable suggestion below?)

Also, should we instead prefer the use of ssh over https? I feel like I once read that GitHub was encouraging ssh over https, but I might be wrong...

(That said, if we recommend ssh, we may want to point people at this GitHub page for help getting set up)

[aaron-hopkinson]

NB: The README.md file in this repo recommends ssh

[VeraChristina]

See above (as to package vs core)
I haven't changed to ssh for now -- different question: does this even make sense given that this is the public documentation and we are assuming people are working on forks?

[aaron-hopkinson]

Good point - copying over my comment from Teams for visibility to others.

I think if you want to cover the forked repo workflow, you could either: run through the GitHub forking process, then tell people to clone their fork, and then set up upstream as a remote, and then explicitly set no_push on upstream. Or: clone upstream, and if people want to fork, set up the fork as a remote.

[gmertes]

For consistency we should recommend semantic commit keywords, so feat/your-feature

[aaron-hopkinson]

Challenge: if it's on a personal fork, does it matter?

[VeraChristina]

No, but also doesn't hurt to at least keep the example consistent -- which it now is. (I think this is one of several places where we don't exclusively have the "fork case" in mind. But I also don't think that this particular bit overburdens users -- happy for people to disagree.)

[gmertes]

While this is true, it can be confusing to talk about commits to main when contribors don't commit to main directly.

Instead, I would say that it's only enforced for the PR titles. You already say this in the next section, but I would reiterate it here.

I would maybe even not talk about "commit guidelines" at all, but only talk about "PR title guidelines". Since the commits that users do inside their own branch don't really matter at all imo. When you review PRs you just look at the code changes, not at the commit messages.

[aaron-hopkinson]

More flexibility in commit messages is something I approve of. I think the fewer rules we impose before things get to the PR/merging stage, the better - at least in terms of reducing the barriers to contributing.

[aaron-hopkinson]

But I can absolutely see why having some enforcement around the PR merge-sqaush-commit is helpful :)

[VeraChristina]

Very good point. Have changed updated the "commit guidelines" (now "PR title guidlines") accordingly

[VeraChristina]

On Aaron's point, the wording is now that we "recommend" this for commits more generally -- hope that leaves enough room (as it's under PR title guidelines, not commit guidelines)

[aaron-hopkinson]

When developing on your own, I like to commit very often on my local clone, so I don't lose my work and I can stash when things get bad. Usually, I don't want to expose all these drafts to the public view, so, I git rebase -i to play around with my commits until I have something I actually want to share, and only then I push.

@floriankrb I do this too, and think it's good practice to try to clean up your git history before creating a PR (although I'm not always perfect at this...). Once the code's public, I think we should generally try to avoid rewriting history/force pushing in order to avoid inconveniencing other people.

That said, draft PRs are a good way of getting feedback while the code is still being developed (and something we encourage in the docs) - and are explicitly not in a mergeable state - so maybe we could say that you're free to do whatever you want on a draft PR, but if it's a proper PR (i.e. intended to be merged), then we probably shouldn't be rewriting history from that point on? That seems like a sensible balance to me. What do you think?