-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revamp the contributor guide #473
Revamp the contributor guide #473
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see more reviewers for this.
I‘ll have to review the branch selection rules later, with a clear mind. In general, I'd say there is a lot of information in this chapter that should be routine for people familiar with Do we really need to provide all that? Do we anticipate contributions from users who haven’t done all that before? |
db7abfa
to
a10c250
Compare
There's indeed pretty basic stuff, so we could remove more than I already did. That being said, here are the most common issues I see:
|
ping @mpdude 🙂 |
I personally use origin for the official repository. Let us avoid "origin" entirely, as it might lead to some confusion.
Not everyone wants to contribute directly, some people might want to toy with the repository before deciding to fork.
Still have to go through the textual changes. Regarding the other points you brought up: When we ask people to re-target PRs, can we expect/assume they find these instructions? Would it help if we set up a canned reply in GitHub, so that when we ask people to change the target branch we could include a pointer to this documentation? Regarding bad commit messages, meaningless commits etc.: Would it make sense to discuss squash-merging commits like Symfony does? That would help to hide “how the sausage is being made” and have a clean history with one commit per change (= PR). |
1515a5e
to
7bc40bc
Compare
source/contribute.rst
Outdated
|
||
The general rules are as follows: | ||
|
||
- Patch releases should be as stable as possible. They fix bugs, but in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "in"
source/contribute.rst
Outdated
If you are touching something outside the source directory (usually | ||
``src``, sometimes ``lib``), and that thing is not ``composer.json``, | ||
you should probably target the lowest branch. | ||
There is no way that your contribution is going to create a bug for | ||
the end user, so let us get it on all 3 branches with merges up. | ||
Examples are: documentation, tests, build scripts, tooling configuration. | ||
|
||
If you are touching something inside the source directory, you may | ||
still contribute to the lowest branch if it is a bug fix, and if you | ||
are touching comments that are not phpdoc comments. | ||
|
||
If the comments are phpdoc comments… it depends. If the phpdoc was | ||
inaccurate, you should target the lowest branch. If the phpdoc was | ||
imprecise but technically correct, you should target the next minor | ||
branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things that can go on the patch release branch include:
- Bugfixes 🥹
- Adding tests, especially for bugs fixed
- Updates, corrections or improvements to non-code assets like documentation, build scripts or tooling configuration
- Updates, corrections or improvements to code comments that are not phpdoc comments (docblock type hints etc.)
- Fixes to incorrect phpdoc comments
- When phpdoc comments are imprecise but not wrong technically, target the next minor release branch instead.
source/contribute.rst
Outdated
If you are doing a refactoring and you think you didn't change the | ||
behavior of the code, you should still target the next minor branch | ||
unless the refactoring is necessary for a bugfix. Let us not take any | ||
unnecessary risks. | ||
|
||
If you want to deprecate something, you should target the next minor | ||
branch: upgrading to a new patch version should not result in any | ||
additional deprecations. | ||
|
||
If you want to add a new feature, you should target the next minor | ||
branch, because it might have unintentional side effects that break existing code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next minor version branch may include:
- Refactorings, unless they are necessary for bugfixes. This is to avoid unnecessary risks.
- Adding deprecations – remember to update the
UPGRADE.md
file as well! - Adding new features and/or public APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about changing minimum dependency requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing minimum dependency requirements is OK on a patch branch if it's absolutely necessary to fix a bug. I don't think that's often the case, and I think we don't have to mention this here.
source/contribute.rst
Outdated
If you want to remove or change a public API, you should target the next | ||
major branch. If you change a public API, you should provide an upgrade | ||
path for the end user. Ideally, you should make the new API available on | ||
the next minor branch, deprecate the old API, and remove it in the next | ||
major. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the next major version branch, usually only deprecations notices, deprecated features and necessary compatibility layers are being removed.
Avoid suprising changes in public APIs on the next major version branch if anyhow possible. In order to provide a smooth upgrade path to users, the preferred solution is to add deprecations and possibly alternative APIs in a preceding minor version, and only remove the deprecations and compatibility layers in the next major release.
source/contribute.rst
Outdated
1. Fetch all new commits: `git fetch doctrine``. | ||
2. Rebase on what you fetched interactively: | ||
``git rebase --interactive doctrine/2.3.x # use -i for short`` | ||
3. A window will show up with many lines, remove every line but the last | ||
ones, which should correspond to your commits. | ||
4. If you run into a conflict, fix it with ``git mergetool``, then | ||
continue on your merry way with ``git rebase --continue``. | ||
5. Force push to overwrite the previous version : ``git push --force``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebases will lose approvals. Just sayin'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paragraph is "typically because the build had an issue", and I personally don't approve pull requests that have a red build.
7bc40bc
to
f06ca12
Compare
source/contribute.rst
Outdated
must be available in the minor release that adds the deprecation for | ||
the old feature. | ||
|
||
With that in mind: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we have two sentences that end with a colon
about that | ||
<https://docs.github.com/en/get-started/quickstart/contributing-to-projects>`_ | ||
first. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When following that documentation, you'll end up with a clone of your fork, so origin
refers to that. Also, in these instructions, only the default branch from the base repo will be included in the clone.
So, maybe we should add instructions here to add doctrine
as a second remote pointing to the upstream (original) Doctrine repo, and to pull all branches from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or to rename it… should be less confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember, we are writing this mostly for an audience with little to no experience in contributing on GitHub. So we should try to leave them with a setup that is as “mainstream” as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that there is a mainstream setup. Some people use origin
for their forks, some other for the upstream repository… I don't have statistics on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of origin
pointing to forks: https://symfony.com/doc/current/contributing/documentation/overview.html#your-first-documentation-contribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or to rename it… should be less confusing.
I think I misunderstood this, I thought you wanted to rename origin
to something else.
I agree, there are people who clone the original repo and add their fork as a second remote, or they clone their fork and add upstream
or similar.
Either way, wouldn't it be safe if we suggest to add a doctrine
remote pointing to the original Doctrine project's repo, and use that remote name in all other git commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just see that in fact, you do that – rename origin
to doctrine
. That may be wrong, depending on how people set up their clone. And if they follow the GitHub instruction linked above, it will definetly be wrong, since those instructions end up with origin
= the fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greg0ire I think this point still needs to be addressed:
Simply renaming origin
to doctrine
may not be correct. Depending on how people obtained their clone, origin
may be their fork or the original (upstream) repo.
When closely following the instructions linked in the previous sentence, then origin
will be the fork, so the rename suggested here is definetly wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I addressed it now. I suggested starting afresh with a git remote rm
source/contribute.rst
Outdated
Time might have elapsed since the last time you contributed or since you | ||
cloned the repository. You might want to fetch the latest changes from | ||
the ``doctrine`` remote: | ||
|
||
.. code-block:: console | ||
|
||
$ git fetch doctrine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move that a bit down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree, see #473 (comment)
source/contribute.rst
Outdated
------------------------- | ||
|
||
New pull requests are created with the repository's default branch as | ||
base branch, and that might not be what you want. Make sure to pick the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base branch should be what you chose according to the guidelines mentioned in the beginning of this chapter. It is also the branch name that you provided when you created your topic branch.
source/contribute.rst
Outdated
New pull requests are created with the repository's default branch as | ||
base branch, and that might not be what you want. Make sure to pick the | ||
correct branch when creating the pull request. If you do not, it can | ||
still be changed afterwards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'a a bit hidden – on GitHub, you have to click "Edit" next to the pull request title to change the title and base branch
.. code-block:: console | ||
Sometimes, you will need to rebase your branch on the latest changes, | ||
typically because the build had an issue unrelated to your changes, and | ||
that issue has been fixed after you created your branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, a ebase takes all changes on your topic branch and moves them to another starting point. This starting point was the Doctrine branch that you chose when you created your topic branch, at that point in time. The rebase will move your changes to be based on the current state of this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
source/contribute.rst
Outdated
3. A window will show up with many lines, remove every line but the last | ||
ones, which should correspond to your commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that's pretty vague, and every line removed potentially is a lost commit. A nice footgun, eh?
As long as you're only rebasing on the same branch that you initially started out with, does it have to be an interactive rebase at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Assuming we're writing all this for people rather new to git and contributing on GitHub, so we should not talk them into complicated situations where they might lose work and/or get frustrated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you're only rebasing on the same branch that you initially started out with, does it have to be an interactive rebase at all?
That's the thing, newbies will often pick the wrong branch, and might have to rebase on another branch, and commit they don't own. I'll reword with "commits that you did not author"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah but the paragraph is not about this use case. You're right, let's make this non-interactive, I've been mixing things up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s have a dedicated section about changing the target branch and the rebase operations that are necessary for this.
Additionally, prepare a canned reply that we can use in PRs whenever we ask people to change the target branch. This should point to that documentation, mention the correct branch and tell people that they need not close the PR but can update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this is already in a very good shape, keep going!
Added a next round of feedback – sorry I missed some of the points last time.
Regarding #473 (comment)
IMHO squash-merging PRs could solve all those problems. We could also ask contributors to keep the initial PR description an up-to-date description and summary of the change and use that as the commit message for the merge. Symfony does it that way as well. Where and how could this best be discussed with the relevant decision-makers from the Doctrine core team? |
The default branch is the branch you see when you enter the repository | ||
page on GitHub. | ||
|
||
.. image:: ../images/default-branch.png | ||
:alt: The default branch | ||
:style: margin-bottom: 20px | ||
|
||
In this DBAL example, it's the branch with the name **2.11.x**. The | ||
branch name reflects the current lowest supported version of a | ||
repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking if this sentence is relevant, or do you want me to add relevant information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
source/contribute.rst
Outdated
|
||
Sync your local 2.11.x branch: | ||
1. Fetch all new commits: `git fetch doctrine``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Switch to the branch you'd like to rebase.
source/contribute.rst
Outdated
1. Fetch all new commits: `git fetch doctrine``. | ||
2. Rebase on what you fetched: | ||
``git rebase doctrine/2.3.x`` | ||
3. If you run into a conflict, fix it with ``git mergetool``, then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never used git mergetool
– does that include git add
for the resolved conflicts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'll try to rework this.
source/contribute.rst
Outdated
against ``2.3.x`` but you are told to change it to ``2.4.x``. In that | ||
case, if ``2.4.x`` does not have all the commits that ``2.3.x`` has, you | ||
need to drop these commits while rebasing: it is not your responsibility | ||
to merge them up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I was not aware of that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, subconsciously I was.
git checkout my-topic
git rebase doctrine/2.3.x --onto doctrine/2.4.x
No need to pick the right commits. Am I missing something?
I should read your full changes before jumping in, you've mentioned this below.
Isn't that much easier and safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably right, let's just suggest that one method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using that method, the "commits you did not author" thingy is not a issue at all, right? If so, we need not mention it.
Another case where you need a rebase is when you want to change the
target branch of your PR. For instance, you might have created your PR
against 2.3.x
but you are told to change it to 2.4.x
. In that
case, the following command will pick all changes that you made against the 2.3.x
branch, and re-apply them on the current 2.4.x
branch.
$ git fetch doctrine
$ git rebase --onto doctrine/2.4.x doctrine/2.3.x your-topic-branch
$ git push --force
After that, you also need to update the GitHub pull request to point to the new target branch. You can do so by clicking on the "Edit" button next to the pull request title. After you changed the target branch, the pull request should only show your commits and changes, but this time they are based on the new target branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using that method, the "commits you did not author" thingy is not a issue at all, right? If so, we need not mention it.
Not an issue indeed 👍
``rebase --continue``. At any stage, you can abort the rebase with | ||
``rebase --abort`` unlike nasty merges which will leave files strewn | ||
everywhere. | ||
On upstream changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"on" or "onto"? Not a native speaker myself. Git CLI suggests onto
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From man git rebase
: "it may not be the best idea to keep rebasing on top of the upstream"
source/contribute.rst
Outdated
``drop``. | ||
4. If you run into a conflict, fix it with ``git mergetool``, then | ||
continue on your merry way with ``git rebase --continue``. | ||
5. Force push to overwrite the previous version : ``git push --force``. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Update the Pull Request target in GitHub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already mentioned above (and I think it's best to do it before pushing)
Sorry for all the nitpicking, I owe you a beer once we happen to meet IRL |
e4bfc57
to
34c23d0
Compare
Can't wait for the next EU-FOSSA hackathon! |
Oh good ol' times |
@mpdude IMO this is ready, can I get a formal approval from you? @SenseException , would it be sufficient for you or do you wish to get other maintainers to review this? I'd hate to let this rot… |
Yes, @mpdude fits perfectly as second reviewer. His approval should make this getting merged. 👍 |
source/contribute.rst
Outdated
When cloning, you will end up with a remote called ``origin``. This can | ||
be confusing because sometimes, people use ``origin`` to refer to their | ||
fork. | ||
|
||
To avoid confusion, we recommend you to rename the ``origin`` remote to | ||
``doctrine``: | ||
|
||
.. code-block:: console | ||
|
||
$ git clone [email protected]:username/dbal.git | ||
$ git remote rename origin doctrine | ||
|
||
- Enter the dbal directory and add the **doctrine** remote | ||
Also, please ensure your Github fork is known as ``fork`` locally: | ||
|
||
.. code-block:: console | ||
|
||
$ cd dbal | ||
$ git remote add doctrine git://github.com/doctrine/dbal.git | ||
$ git remote add fork [email protected]:<your-name-here>/orm.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion for this entire section:
To make the example commands given in the rest of this page less ambiguous, we will add two explicit names for remote Git repositories: `doctrine` for the original Doctrine project you're contributing to, and `fork` for your fork of it.
.. code-block:: console
$ [`cd` into the directory created by `git clone ...`]
$ git remote add doctrine [email protected]:doctrine/<name-of-project>.git
$ git remote add fork [email protected]:<your-github-name>/<name-of-project>.git
source/contribute.rst
Outdated
.. note:: | ||
|
||
Here, we assume ``doctrine`` points to the upstream repository, and | ||
not your fork. If you are not sure, you can check with ``git remote -v``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this note since we've added the remote name in the beginning section.
"Remember, we assume that doctrine
points to the original Doctrine project repository and not to your fork, as described in the initial section of this page. If unsure, you can check with git remote -v
.
source/contribute.rst
Outdated
Finished topic branches should be pushed to **origin** for a | ||
**maintainer** to review and pull into **doctrine** as appropriate: | ||
1. Switch to the branch you would like to rebase. | ||
2. Fetch all new commits: `git fetch doctrine``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this missing a backtick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is 👍
@greg0ire Done. Can you easily follow up on the new comments I made? Otherwise, I'll walk you through them one by one. Not much left – let's push this over the finish line soon! |
34c23d0
to
4c9c51a
Compare
I think I did, but I also think I might have easily missed some comments. Can you mark the resolved ones as such? |
For some reason, I cannot mark (everything) as "resolved" 🤔 . Let's see: |
4c9c51a
to
f75f309
Compare
source/contribute.rst
Outdated
date with the following command: | ||
To make the example commands given in the rest of this page less | ||
ambiguous, we will add two explicit names for remote Git repositories: | ||
`doctrine` for the original Doctrine project you're contributing to, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.rst need double backticks here and in the following lines?
While some sections feel redundant, other sections were missing, like the one about choosing the right branch.
f75f309
to
7302b61
Compare
All comments addressed ✅ . Awesome work @greg0ire 🥇 and sorry it took so many iterations. I really like the result! |
Thanks for the thorough review! |
While some sections feel redundant, other sections were missing, like
the one about choosing the right branch.
This only addresses the first question of #472
@mpdude please review.