Add guidelines on submitting batch commits #55
Conversation
IwoHerka
requested review from
maticomp,
Struchu,
dragonee,
BarryOgg,
MajekX,
rkowalski4,
kaniak274 and
KamilSupera
| @@ -195,6 +195,26 @@ Fix nasty little bug ISSUE-51 #resolve #time 2h30m | |||
| * Keep the history *clean* and *simple*. Before pushing any branch to the | |||
| public always ensure that it conforms to the style-guide. If it doesn't: | |||
| squash, rebase, reword commits as necessary. | |||
|
|
|||
| * When merging multiple small commits into a single change, make sure to | |||
| keep high locality of the changes. In another words, if you're changing, | |||
There was a problem hiding this comment.
What does "group changes in small vicinity of each other" mean in git commit context?
There was a problem hiding this comment.
It means changes in the same function or same class or same logical unit of the code. The goal here is to disqualify groupings which include changes happening all over the place.
There was a problem hiding this comment.
Hmm, I would use a real life example here as it is not as clear in the text.
| @@ -195,6 +195,26 @@ Fix nasty little bug ISSUE-51 #resolve #time 2h30m | |||
| * Keep the history *clean* and *simple*. Before pushing any branch to the | |||
| public always ensure that it conforms to the style-guide. If it doesn't: | |||
| squash, rebase, reword commits as necessary. | |||
|
|
|||
| * When merging multiple small commits into a single change, make sure to | |||
There was a problem hiding this comment.
I'm not sure about the word "merging", which for git has a very precise meaning. Do you mean "squashing", or do you mean "posting them for review in a single pull request containing multiple commits"?
There was a problem hiding this comment.
The second one. One a technical level, they can be merged, squashed, amended of rebased. I will change the wording.
| let's say, some elements of the UI, group changes | ||
| that occur in small vicinity of each other. | ||
|
|
||
| * When merging multiple changes into one, make sure to _do not exceed |
There was a problem hiding this comment.
Same issue with wording here - do you mean "When commiting multiple changes at once, make sure..."?
| * Fix alpha layer on the toolbar PROJ-19 | ||
| ``` | ||
|
|
||
| where `PROJ-10` is a super-ticket for all subsequent issues. |
There was a problem hiding this comment.
Does this imply that it's valid only when PROJ-10 is a task, and PROJ-13/22/19 are subtasks, or can these issues be logically related, but not related in JIRA?
What I'm trying to say is that I feel the guide applies well to any batches of multiple changes, not necessarily strictly related by JIRA issues.
There was a problem hiding this comment.
I'm leaving here a little wiggle room. Sometimes there will be a supertask, and sometimes not (depends on the QA most of the time). The implicature of the guide is that if there are tickets, include them, and if not, consider making them. Ultimately, the choice is of the developer.
| let's say, some elements of the UI, group changes | ||
| that occur in small vicinity of each other. | ||
|
|
||
| * When merging multiple changes into one, make sure to _do not exceed |
There was a problem hiding this comment.
Myślałem że mamy po jednej zmianie zazwyczaj na commit?
There was a problem hiding this comment.
Mamy, ale batch commity zdarzaja sie i beda sie zdarzac. Realistycznie, nie wymagalabym od nikogo osobno commitowania zmiany paddingu. To by wrecz zasmiecalo repo. Jezeli ktos ma pare drobnych poprawek ktore sa ze soba logicznie powiazane, to chcialbym batch commit.
There was a problem hiding this comment.
To dodałbym "do that only with changes that do not exceed a few lines"
| ticket/issue numbers in the commit log. For example: | ||
|
|
||
| ```git | ||
| Improve user-experience in subscription views PROJ-10 |
There was a problem hiding this comment.
Personally I'm not in favour of creating commits that relate to several JIRA issues. For me it's fogging the boundaries dividing changes that are related to specific tasks/bugs etc.
There was a problem hiding this comment.
See me response to @dragonee above.
There was a problem hiding this comment.
I see the point, but I would emphasize that this is not recommended and should be done as a last resort: otherwise people will abuse this rule.
There was a problem hiding this comment.
On the other hand - sometimes tickets are created in a very granular way (e.g. separate ticket for each letter of CRUD), and so there no logical way to separate them, and so a single commit is related to at least four tickets, in a way that doesn't seem last resort-ish to me
There was a problem hiding this comment.
"separate ticket for each letter of CRUD" - This sounds to me like 4 pull requests for 4 features.
There was a problem hiding this comment.
To be completely clear, this proposition is about really small commits.
| let's say, some elements of the UI, group changes | ||
| that occur in small vicinity of each other. | ||
|
|
||
| * When merging multiple changes into one, make sure to _do not exceed |
There was a problem hiding this comment.
I think "make sure to do not" is not gramatically correct. Should rather be "make sure not to".
| @@ -195,6 +195,26 @@ Fix nasty little bug ISSUE-51 #resolve #time 2h30m | |||
| * Keep the history *clean* and *simple*. Before pushing any branch to the | |||
| public always ensure that it conforms to the style-guide. If it doesn't: | |||
| squash, rebase, reword commits as necessary. | |||
|
|
|||
| * When merging multiple small commits into a single change, make sure to | |||
There was a problem hiding this comment.
I think we shouldn't merge/squash multiple small commits into one if they correspond to different tickets. imo, every commit should correspond to one ticket if possible. We could add summary as shown below in pull request overview instead.
There was a problem hiding this comment.
We could use approach you suggested but we should change merge strategy.
There was a problem hiding this comment.
I'm not convinced that anyone will have the self-discipline to do so. This proposition relaxes git guidelines a bit to compensate for really small changes. Making separate commits in those cases is cumbersome and not really helpful anyway. Right now, for example, we have a rule to amend pull request instead of adding "Apply review changes" commit. Do you obey this rule? Adding more and more strict rules is not realistic.
There was a problem hiding this comment.
I don't think that overview filling is more strict than commit squashing.
To be clear: I am not against you approach, I just think than pr overview is good enough.
I'm adding new guideline for batch commits. At the moment, batch comments tend to be very messy and hard to read for the reviewers.