[Notes] [Git][BuildStream/buildstream][master] 4 commits: contributing: WIP again to be kind to reviewers

[Angelos Evripiotis <gitlab mg gitlab com>]

Title: GitLab

Angelos Evripiotis pushed to branch master at BuildStream / buildstream

Commits:

• c0a8bb66
  by Angelos Evripiotis at 2018-11-08T15:49:16Z

  contributing: WIP again to be kind to reviewers
  
  As someone coming from GitHub to GitLab, I was pleasantly surprised by
  the 'filter by WIP status' option.
  
  To make sure we get the most out of it, add a guideline to keep the
  filter clean, to reduce reviewer burden.
  

• f7231e90
  by Angelos Evripiotis at 2018-11-08T15:49:16Z

  contributing: non-WIP should always be landable
  
  I was recently surprised that we don't prefer addressing review comments
  in 'fixup!' commits.
  
  Coming from GitHub, I've found that fixup commits make it easier for
  reviewers to see what has changed since their last review. The idea is
  to use '--autosquash' before landing to clean up the history again.
  
  It's a pleasant surprise that it's easy to keep track of what changed in
  merge-requests between pushes, so we can always keep the history clean.
  
  Document this, so folks like me can see the light sooner.
  

• f7643440
  by Angelos Evripiotis at 2018-11-08T15:49:16Z

  contributing: emphasise the 'why' in commits
  
  Explain why it's useful to mention the decisions made for a change and
  provide links for background, e.g. issue numbers.
  
  My general experience of git histories is that folks don't do enough
  explaining, so it's great we have a contributing section for it.
  
  I've found it's easier for folks to stick to rules when they know the
  practical reasons for them, so provide some.
  

• dd5e7b04
  by Angelos Evripiotis at 2018-11-08T16:17:04Z

  Merge branch 'aevri/contributing_gitlab' into 'master'
  
  Add more to GitLab-relevant parts of contributing
  
  See merge request BuildStream/buildstream!935

1 changed file:

• CONTRIBUTING.rst

Changes:

CONTRIBUTING.rst

---

...	...	@@ -97,7 +97,13 @@ a new merge request. You can also `create a merge request for an existing branch
97	97	You may open merge requests for the branches you create before you are ready
98	98	to have them reviewed and considered for inclusion if you like. Until your merge
99	99	request is ready for review, the merge request title must be prefixed with the
100		-``WIP:`` identifier.
	100	+``WIP:`` identifier. GitLab `treats this specially
	101	+<https://docs.gitlab.com/ee/user/project/merge_requests/work_in_progress_merge_requests.html>`_,
	102	+which helps reviewers.
	103	+
	104	+Consider marking a merge request as WIP again if you are taking a while to
	105	+address a review point. This signals that the next action is on you, and it
	106	+won't appear in a reviewer's search for non-WIP merge requests to review.
101	107
102	108
103	109	Organized commits
...	...	@@ -122,6 +128,12 @@ If a commit in your branch modifies behavior such that a test must also
122	128	be changed to match the new behavior, then the tests should be updated
123	129	with the same commit, so that every commit passes its own tests.
124	130
	131	+These principles apply whenever a branch is non-WIP. So for example, don't push
	132	+'fixup!' commits when addressing review comments, instead amend the commits
	133	+directly before pushing. GitLab has `good support
	134	+<https://docs.gitlab.com/ee/user/project/merge_requests/versions.html>`_ for
	135	+diffing between pushes, so 'fixup!' commits are not necessary for reviewers.
	136	+
125	137
126	138	Commit messages
127	139	~~~~~~~~~~~~~~~
...	...	@@ -144,6 +156,16 @@ number must be referenced in the commit message.
144	156
145	157	Fixes #123
146	158
	159	+Note that the 'why' of a change is as important as the 'what'.
	160	+
	161	+When reviewing this, folks can suggest better alternatives when they know the
	162	+'why'. Perhaps there are other ways to avoid an error when things are not
	163	+frobnicated.
	164	+
	165	+When folks modify this code, there may be uncertainty around whether the foos
	166	+should always be frobnicated. The comments, the commit message, and issue #123
	167	+should shed some light on that.
	168	+
147	169	In the case that you have a commit which necessarily modifies multiple
148	170	components, then the summary line should still mention generally what
149	171	changed (if possible), followed by a colon and a brief summary.

—
View it on GitLab.
You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can adjust your notification settings.