5 ms·

> Proper style? Formatting. Imperative voice. Capitalization. Hard wraps. Correct, those don't matter much. On the other hand, describing what the change is ab

> Proper style? Formatting. Imperative voice. Capitalization. Hard wraps. Correct, those don't matter much. On the other hand, describing what the change is about matters a lot. It is 10x easier to review a merge request when you can see discrete steps that make sense (ex: update method X, update route Y that calls X, update all code paths that use route Y, update client to handle new data) vs one big jumble of "stuff", "fix it", "update getCustomers" that touches 27 places at once.

TheSoftwareGuy 4y ago
To counter, any merge request with more than 2-3 commits is probably too big anyway
- ricardobeat 4y ago
  Not a fan of those arbitrary limits. It's all about being able to quickly understand the changes and their reason to be. If splitting the updates in smaller increments help, then so be it, and I'll take ten tidy small commits over two large ones any day.
  - morelisp 4y ago
    PR guidelines - Git best practices generally - are one of those situations where you must provide concrete guidelines but also never, ever make them a hard rule. If you don't have any you'll get endless "fix" "oops" +9876/-1234 changes, and if you make them a hard rule you'll never get anything useful done as people spend all day trying to please the aesthetic preference of a team lead who probably hasn't written a more than a single two line commit in years.
- wizofaus 4y ago
  Huh? The number of commits is irrelevant - the number of changed lines/ files is what matters. 20 commits can result in a couple of lines in one file changing (I've seen it happen) and 1 commit can have 100s of files changed (usually when some commonly used symbol has been renamed).
nawgz 4y ago
If the update is going to touch 27 places at once, how is the developer meant to communicate such a change thru commit messages?
- ricardobeat 4y ago
  By grouping the changes logically (by system, context, feature, up to you), as in the example I gave. Sometimes it's as simple as putting backend & frontend changes into separate commits for easier review. Of course you still need an overarching description in the merge request - should be really easy to write, when you have all the steps taken right in front of you.
  - staticassertion 4y ago
    This assumes that people work in logical, sequential chunks. I don't. I do a little here, a little there, and then it all starts to form the larger picture.
    - ricardobeat 4y ago
      Not necessarily, I don't think anyone works linearly, but you can still keep your commits organized. I might have 25 dirty files and a bunch of WIP commits; when I'm done I will reset everything, and take the time to stage them one by one (or `git add -p`) into commits that make sense, and will help a reviewer understand what's happening. It's worth the extra few minutes. For a more concrete example, say you're adding a new screen to a mobile app, I'll probably split into chunks like these: - add new `/foo` endpoints to API - add `NewScreen` (can be reviewed as a feature, does the screen contain / do what it's suppose to) - add `NewScreen` to navigation stack - link to `NewScreen` from settings page - add tracking for `NewScreen` Each of these have very different purposes, and I find being able to focus on the different aspects (business logic, infrastructure stuff, analytics) helps a lot. They can also be reverted cleanly if needed without destroying all of the work. You can still review the entire PR at once, with the option to drill down into individual commits if desired.

TheSoftwareGuy 4y ago

To counter, any merge request with more than 2-3 commits is probably too big anyway

ricardobeat 4y ago
Not a fan of those arbitrary limits. It's all about being able to quickly understand the changes and their reason to be. If splitting the updates in smaller increments help, then so be it, and I'll take ten tidy small commits over two large ones any day.
- morelisp 4y ago
  PR guidelines - Git best practices generally - are one of those situations where you must provide concrete guidelines but also never, ever make them a hard rule. If you don't have any you'll get endless "fix" "oops" +9876/-1234 changes, and if you make them a hard rule you'll never get anything useful done as people spend all day trying to please the aesthetic preference of a team lead who probably hasn't written a more than a single two line commit in years.
wizofaus 4y ago
Huh? The number of commits is irrelevant - the number of changed lines/ files is what matters. 20 commits can result in a couple of lines in one file changing (I've seen it happen) and 1 commit can have 100s of files changed (usually when some commonly used symbol has been renamed).

nawgz 4y ago

If the update is going to touch 27 places at once, how is the developer meant to communicate such a change thru commit messages?

ricardobeat 4y ago
By grouping the changes logically (by system, context, feature, up to you), as in the example I gave. Sometimes it's as simple as putting backend & frontend changes into separate commits for easier review. Of course you still need an overarching description in the merge request - should be really easy to write, when you have all the steps taken right in front of you.
- staticassertion 4y ago
  This assumes that people work in logical, sequential chunks. I don't. I do a little here, a little there, and then it all starts to form the larger picture.
  - ricardobeat 4y ago
    Not necessarily, I don't think anyone works linearly, but you can still keep your commits organized. I might have 25 dirty files and a bunch of WIP commits; when I'm done I will reset everything, and take the time to stage them one by one (or `git add -p`) into commits that make sense, and will help a reviewer understand what's happening. It's worth the extra few minutes. For a more concrete example, say you're adding a new screen to a mobile app, I'll probably split into chunks like these: - add new `/foo` endpoints to API - add `NewScreen` (can be reviewed as a feature, does the screen contain / do what it's suppose to) - add `NewScreen` to navigation stack - link to `NewScreen` from settings page - add tracking for `NewScreen` Each of these have very different purposes, and I find being able to focus on the different aspects (business logic, infrastructure stuff, analytics) helps a lot. They can also be reverted cleanly if needed without destroying all of the work. You can still review the entire PR at once, with the option to drill down into individual commits if desired.