Update README with new contribution guidelines #5
Conversation
|
Ehi @sfarrens , would you mind having a look at this? |
|
I will be happy to. 🙂 |
sfarrens
left a comment
There was a problem hiding this comment.
Hi @marcobonici, here are my initial comments on the contribution guidelines. Really nice work! 👏 I think just a bit of effort is needed to clean up the wording and make sure the roles/responsibilities are clear.
colprac/README.md
Outdated
| This document describes some best practices for collaborating on the cloe-org repositories. | ||
| Following these practices makes it easier for contributors (new and old) to understand what is expected of them. | ||
| It should be linked to in the README.md. Those are meant to be general guidelines; it is possible that some repositories have additional guidelines. | ||
|
|
||
| There are many good practices that this document does not cover. | ||
| These include other members of the wider community reviewing pull requests (PRs) they are interested in, and maintainers encouraging and supporting people who open issues to make PRs to solve them. | ||
| This document facilitates these other good practices by clarifying what can seem like a mysterious process to those who are unfamiliar with it. |
There was a problem hiding this comment.
I feel like the wording in general could use some work, but particularly in this section. I am happy to go into detail, but I am pretty sure even a quick pass with e.g. ChatGPT would clear this up.
There was a problem hiding this comment.
Rephrased, using perplexity.
colprac/README.md
Outdated
|
|
||
| #### Contributing PRs | ||
|
|
||
| * PRs should match the existing code style present in the file. |
There was a problem hiding this comment.
Are there global style guidelines for CLOE?
There was a problem hiding this comment.
So,
these are meant to be the general guidelines for the cloe-org. It can (and will) happen that each repository has specific guidelines.
There was a problem hiding this comment.
This has been written at the very beginning of the text, so that it's clear.
colprac/README.md
Outdated
|
|
||
| * PRs should match the existing code style present in the file. | ||
| * PRs affecting the public API, including adding new features, must update the public documentation. | ||
| * Comments and (possibly internal) docstrings should make the code accessible. |
There was a problem hiding this comment.
Modified as discussed with Sam.
colprac/README.md
Outdated
| * PRs affecting the public API, including adding new features, must update the public documentation. | ||
| * Comments and (possibly internal) docstrings should make the code accessible. | ||
| * PRs that change code must have appropriate tests. | ||
| * Changes to the code must be made via PR, not pushing to master. |
There was a problem hiding this comment.
'master' -> 'main', also would it not be wise to protect the main branch such that it is not possible to directly push anyway?
There was a problem hiding this comment.
Ok, so your suggestion is that PR can be made only to develop and that we (the mantainers) push to main when appropriate. Is this right?
There was a problem hiding this comment.
ALso, I created a rulest for the main
https://github.com/cloe-org/cloelib/settings/rules/4372746
I cannot enforce it yet as the repo is private and we do not enterprise. Let me knwo whether you think it makes sense or need something else (I just enforced the bare minimum from my side).
There was a problem hiding this comment.
Not exactly, depending on how many people are contributing to a package and what the release strategy is, it is not unreasonable for PRs to target main. My point is that no one should be 'pushing' to main (or develop), all changes should come from PRs that 'merge' into the appropriate branch. I would propose that you explicitly protect these branch so that no one can push to them.
There was a problem hiding this comment.
The rule ensures that the only way to commit to main is by PRs. Or am I missing something?
colprac/README.md
Outdated
| * Comments and (possibly internal) docstrings should make the code accessible. | ||
| * PRs that change code must have appropriate tests. | ||
| * Changes to the code must be made via PR, not pushing to master. | ||
| * Before starting to work on a PR, especially when including or modifying a huge chunk of code, discuss with the mantainers the change you are gonna implement and how. |
There was a problem hiding this comment.
Are the maintainers for each repo listed in the README.md?
There was a problem hiding this comment.
So, this is something to discuss with the others (@gcanasherrera @PedroCarrilho @chiaramoretti @AndreaPezzotta ).
For the moment, mantainers are in charge of the whole organization. Something different might happen in the future!
colprac/README.md
Outdated
|
|
||
| * A release should be made as soon as possible after a bugfix PR is merged. | ||
| * Care and consideration should be given as to when to make a breaking release. | ||
| * The person who merged the PR should register the new release of the package. |
There was a problem hiding this comment.
If the person is a repo maintainer, this makes sense. You may want to add a couple of bullet points on what the general strategy for major vs minor releases.
There was a problem hiding this comment.
Added link to semver.
colprac/README.md
Outdated
| * PRs with large improvements to style should not also change functionality. | ||
|
|
||
| * PRs introducing breaking changes should make this clear when opening the PR. | ||
| * You should not push commits with commented-out tests. |
There was a problem hiding this comment.
I would argue that nothing should be pushed with commented out code (tests or whatever). There is no reason for this to be in the released code.
There was a problem hiding this comment.
I modified according to the suggestion
| * Small review suggestions, such as typo fixes, should make use of the `suggested change` feature. | ||
| * This makes it easier and more likely for all the smaller changes to be made. | ||
| * Reviewers should continue acting as reviewers until the PR is merged. | ||
|
|
There was a problem hiding this comment.
Just a general and personal points, I would stress that the PR process is intended to be a conversation to converge on a solution that improves the existing code. Developers should not take comments/questions personally and should be prepared to justify the implementation choices they have made. Similarly, reviewers should ask for clarification if a given implementation choice is not obvious/transparent.
There was a problem hiding this comment.
Done as suggested.
| * This makes it easier and more likely for all the smaller changes to be made. | ||
| * Reviewers should continue acting as reviewers until the PR is merged. | ||
|
|
||
| ### Accidental breaking releases |
There was a problem hiding this comment.
I guess this info is exclusive to repo maintainers, right?
There was a problem hiding this comment.
You mean, the Accidental breaking release?
There was a problem hiding this comment.
Answered as asked by Sam.
There was a problem hiding this comment.
Yes, the point is that general developers should not be the ones dealing with this type of issue.
There was a problem hiding this comment.
Added this information.
Co-authored-by: Samuel Farrens <samuel.farrens@gmail.com>
|
Hi @sfarrens , I think I addressed all of the points. There are still a bunch of open points, where I asked for feedback/suggestions. |
|
Looks good @marcobonici! I replied to the questions you posed and check the changes you made. I think once these final threads are resolved and a global pass on English is made, this is ready to go! |
|
Hi @sfarrens , I think I did everything. Let me know whether there is something missing. |
|
@marcobonici 👍 to merge! |
Hello, I have
Can someone give a look and merge into the main? Maybe @gcanasherrera ?