5 ms·

Give me one example then. One is all it takes to disprove a rule. Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thi

Give me one example then. One is all it takes to disprove a rule. Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).

1dom 1y ago
I agreed with you initially. > I'm fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment created, and the expert finds the way to get the job done. I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality, rather than just doing something because it's best practice and expecting everyone else to do the same. They ultimately get the thing done how they said they would.
- horsawlarway 1y ago
  In my experience - this one is the correct one. Make a commitment, keep the commitment, stay responsible for the commitment afterwards. This whole chain is like arguing on how tidy your desk should be. Some people like it fastidious to the nth degree. Some people prefer a little mess. In neither case does that preference really matter much compared to all the other things a real job entails.
- alganet 1y ago
  > they get everything done as they said they would This. It saves everyone's time.
- MoreQARespect 1y ago
  >I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality I have seen plenty of huge PRs which were more trouble than they were worth to break up after discovery. At some point it becomes like unbaking a cake. It's a trade off. Ive just never thought when I saw any of them that there wasnt a more practical way to get there with a bunch of smaller PRs. Unlike dealing with an already existent large PR this isnt really a trade off thing - there are basically almost no circumstances when it is preferable to review one 1000 line code change instead of 4x self contained 200 line changes.
overfeed 1y ago
> Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies. Sometimes it's easier to split this into smaller chunks and front-load reviews for PRs into the feature branch, and then merge the big change with no further reviews, which may go against some ham-fisted rule about merging to main. Knowing when to break rules and why, ownership, and caring for the spirit of the law and not just the letter are what separates mid-levels from seniors. 1. Or changeset, if your version control system allows stacking.
- status_quo69 1y ago
  You can and should break that up because I'm probably going to want to see screenshots to ensure that the branding changes make sense in context and everything looks consistent. How would you do this? You'd either 1. Create N pull requests then merge all of them together into a big PR that would get merged into mainline at once 2. Do the same thing but do a bit of octopus merging since git merge can take multiple branches as arguments. Since most source control strategies are locked down, this isn't usually something that I can tell my juniors to do The point of breaking things down like this is to minimize reviewer context. With bigger PRs there's a human tendency to try and hold the whole thing in your head at once, even if parts of the pull request are independent from others.
  - overfeed 1y ago
    > The point of breaking things down like this is to minimize reviewer context. This principle is much more important than some rule that says "Merges to main should not be more than 150 lines long". Sticklers for hard-and-fast rules usually haven't achieved the experience to know that adhering to fundamental principles will occasionally direct you to break the rules.
    - ants_everywhere 1y ago
thayne 1y ago
A widely used class has a bad API. You refactor it to make a cleaner API, but your change isn't backwards compatible. Your options are: - Change everything all at once. This creates a large PR. - Split it up into multiple small PRs. Now your individual PRs don't compile, and make less sense on their own. - Create a new class, and then split up multiple PRs that transition code to use the new class, and then finally a PR to remove the old class. This is more work for both the author and the reviewer and the individual PRs are harder to understand on their own.
- mgkimsal 1y ago
  API changes breaking BC feel like they should be using versioning. I don't see enough people putting versioning support in to their API stuff at the outset. I've been chastised for doing that with "YAGNI". And then... one day, we do need it, and trying to introduce versioning support becomes... that much harder.
  - degamad 1y ago
    The context here was a class API, not a REST API, so versioning is not relevant.
    - mgkimsal 1y ago
      You're right... misread that. Does seem like you could namespace classes in to versions. Then it's much clearer which version of a class a caller is using.
- wpollock 1y ago
  This is interesting. I believe one way to deal with such breaking changes is to have multiple PRs, where the breaking change in each is hidden (protected) by a feature flag and tested by unit tests. Once all the PRs are committed, end to end testing can be done by enabling the flag. Any problems in production can be quickly reverted by disabling the flag. Eventually, a final PR removes the now-useless flag. Of course, your mileage may vary; this technique is certainly not suitable for all breaking changes or all workfkows.
BobbyJo 1y ago
Lets say you have an api bug where you are allowing clients to ask for data with a very expensive query, and you want to dissallow that behavior. I think it makes more sense to change both the api spec (remove the endpoint) and the backend (dissallow queries of that kind in the future) in one go. That way you can note in the one PR exactly why both changes are made, referencing whatever bug/postmortem. Making two PRs that separately look like fixes to the issue can be confusing later, and don't really buy you any clarity in the PR itself. Is it possible to break them up? Sure. Is it better to do so? I don't think so. Also, for clarity, neither myself, nor op, every said couldn't be done.
fizzynut 1y ago
A new feature that fundamentally changes the way a lot of code is structured. A group of features that only combined produce a measurable output, but each one does not work without the others. A feature that will break a lot of things but needs to be merged now so that we have time for everyone to work on fixing the actual problems before deadline X as it is constantly conflicting every day and we need to spend time on fixing the actual issues, not fixing conflicts.

1dom 1y ago

I agreed with you initially. > I'm fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment created, and the expert finds the way to get the job done. I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality, rather than just doing something because it's best practice and expecting everyone else to do the same. They ultimately get the thing done how they said they would.

horsawlarway 1y ago
In my experience - this one is the correct one. Make a commitment, keep the commitment, stay responsible for the commitment afterwards. This whole chain is like arguing on how tidy your desk should be. Some people like it fastidious to the nth degree. Some people prefer a little mess. In neither case does that preference really matter much compared to all the other things a real job entails.
alganet 1y ago
> they get everything done as they said they would This. It saves everyone's time.
MoreQARespect 1y ago
>I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality I have seen plenty of huge PRs which were more trouble than they were worth to break up after discovery. At some point it becomes like unbaking a cake. It's a trade off. Ive just never thought when I saw any of them that there wasnt a more practical way to get there with a bunch of smaller PRs. Unlike dealing with an already existent large PR this isnt really a trade off thing - there are basically almost no circumstances when it is preferable to review one 1000 line code change instead of 4x self contained 200 line changes.

overfeed 1y ago

> Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies. Sometimes it's easier to split this into smaller chunks and front-load reviews for PRs into the feature branch, and then merge the big change with no further reviews, which may go against some ham-fisted rule about merging to main. Knowing when to break rules and why, ownership, and caring for the spirit of the law and not just the letter are what separates mid-levels from seniors. 1. Or changeset, if your version control system allows stacking.

status_quo69 1y ago
You can and should break that up because I'm probably going to want to see screenshots to ensure that the branding changes make sense in context and everything looks consistent. How would you do this? You'd either 1. Create N pull requests then merge all of them together into a big PR that would get merged into mainline at once 2. Do the same thing but do a bit of octopus merging since git merge can take multiple branches as arguments. Since most source control strategies are locked down, this isn't usually something that I can tell my juniors to do The point of breaking things down like this is to minimize reviewer context. With bigger PRs there's a human tendency to try and hold the whole thing in your head at once, even if parts of the pull request are independent from others.
- overfeed 1y ago
  > The point of breaking things down like this is to minimize reviewer context. This principle is much more important than some rule that says "Merges to main should not be more than 150 lines long". Sticklers for hard-and-fast rules usually haven't achieved the experience to know that adhering to fundamental principles will occasionally direct you to break the rules.
  - ants_everywhere 1y ago

thayne 1y ago

A widely used class has a bad API. You refactor it to make a cleaner API, but your change isn't backwards compatible. Your options are: - Change everything all at once. This creates a large PR. - Split it up into multiple small PRs. Now your individual PRs don't compile, and make less sense on their own. - Create a new class, and then split up multiple PRs that transition code to use the new class, and then finally a PR to remove the old class. This is more work for both the author and the reviewer and the individual PRs are harder to understand on their own.

mgkimsal 1y ago
API changes breaking BC feel like they should be using versioning. I don't see enough people putting versioning support in to their API stuff at the outset. I've been chastised for doing that with "YAGNI". And then... one day, we do need it, and trying to introduce versioning support becomes... that much harder.
- degamad 1y ago
  The context here was a class API, not a REST API, so versioning is not relevant.
  - mgkimsal 1y ago
    You're right... misread that. Does seem like you could namespace classes in to versions. Then it's much clearer which version of a class a caller is using.
wpollock 1y ago
This is interesting. I believe one way to deal with such breaking changes is to have multiple PRs, where the breaking change in each is hidden (protected) by a feature flag and tested by unit tests. Once all the PRs are committed, end to end testing can be done by enabling the flag. Any problems in production can be quickly reverted by disabling the flag. Eventually, a final PR removes the now-useless flag. Of course, your mileage may vary; this technique is certainly not suitable for all breaking changes or all workfkows.

BobbyJo 1y ago

Lets say you have an api bug where you are allowing clients to ask for data with a very expensive query, and you want to dissallow that behavior. I think it makes more sense to change both the api spec (remove the endpoint) and the backend (dissallow queries of that kind in the future) in one go. That way you can note in the one PR exactly why both changes are made, referencing whatever bug/postmortem. Making two PRs that separately look like fixes to the issue can be confusing later, and don't really buy you any clarity in the PR itself. Is it possible to break them up? Sure. Is it better to do so? I don't think so. Also, for clarity, neither myself, nor op, every said couldn't be done.

fizzynut 1y ago

A new feature that fundamentally changes the way a lot of code is structured. A group of features that only combined produce a measurable output, but each one does not work without the others. A feature that will break a lot of things but needs to be merged now so that we have time for everyone to work on fixing the actual problems before deadline X as it is constantly conflicting every day and we need to spend time on fixing the actual issues, not fixing conflicts.