The right size for a PR

[MonikaFu]

I'm having an existential philosophical PR-related GitHub dilemma and I hope that this is the right channel to phrase it.

How big should a PR be?

I get the appeal of making them very small, almost equivalent to 1 commit. But at the same time I feel like if you split one feature into multiple PRs - you lose context on what they actually are about... I would compare it to building a nuclear bomb in small increments...

Anyone has any opinions?

[jdhoffa]

I definitely don't know the answer, but here's an interesting article advocating for exactly what you suggest, ie. breaking a feature down into smaller sub-features, and having a PR for each: https://medium.com/@hugooodias/the-anatomy-of-a-perfect-pull-request-567382bb6067

Maybe this can spark a conversation

[MonikaFu]

cool! I'm going to read it and see if I feel any more enlightened after.

[jdhoffa]

also yay discussions!!

[AlexAxthelm]

I'm a big fan of "A PR introduces a discrete unit of functionality." So if you're PRing into dev branch, it should be a single, small, self-contained feature. If you're moving dev into main, then it will necessarily include several (hopefully related) features. I think the key thing to keep in mind is "What will break if I revert the merge commit?"

[MonikaFu]

so now the question is: What is a single, small, self-contained feature?

[maurolepore]

I like @AlexAxthelm's answer; it feels general and complete. I think it does contain the answer to "What is a single, small, self-contained feature?" -- the answer seems to be "A single, small, self-contained feature is that which adds a discrete unit of functionality that I can revert without breaking existing code".

For now I would mark Alex's comment as the answer -- just to play with the cool feature "Mark as answer" 🥇

[AlexAxthelm]

hahaha. I think that this mindset also requires us to take a different approach to code modularity. Not to cross the streams, but @cjyetman and I had a good discussion about that (especially in the context of testing) on the 2dii slack last week.

I suppose my quick response to "what is a self contained feature" is: Have you added or improved some functionality, without breaking the contracts that exist between any other parts of the system?

[cjyetman]

Surely there is no correct, one-size-fits-all answer to this. I think maybe a good process might be to think about the reasons that might make you lean towards smaller or bigger. I'll begin... feel free to add/modify...

reasons you might lean towards smaller...

• you need or want someone else to review it
• you would like to cleanly test the impact of the change
• you would like the hypothetical ability to revert the change if something goes wrong with it in the future
• you would like the ability to review the commit history in the future and be able to easily interpret what each merge to master did

reasons you might lean towards bigger...

• a number of files all work together to achieve one goal and you'd like the context of that to be wrapped up into one merge
• the changes in one place do not make sense without the context of changes elsewhere
• it's less hassle and maybe less time for you to figure out how to break it up into smaller pieces

[MonikaFu]

Great points! So yeah, I guess that the context is very important in my mind - but maybe I am wrong about that.

This discussion is in big part triggered by the work we've done on this PR. I understand that it makes it seem easier to review when we broke the PR down into 3 small ones. At the same time - I'm thinking that the 3 separate ones would not make such sense with the context of the entire feature being implemented. But I guess that I should then explain in the PR description that it is a preparation for implementing a certain new feature?

[cjyetman]

totally agree... I'm not 100% convinced that merging a commit that basically has no purpose on its own just as prep for a forthcoming PR is a good idea

[maurolepore]

I think it's great that this PR opened this discussion, to better understand how to work efficiently with PACTA_analysis. But remember the "PACTA" ecosytem is made up of repositories that are not self-contained. That is why that PR ended up split in 3 pieces: You first tried a resonable refactoring step but the braking code forced us to step back and move the same way but in three smaller refactoring steps -- just to keep the system woring at all times.

Those 3 PRs resulted from the trade off between (a) the desire to keep together everything that relates to a single unit of functionality, and (b) the need to brake no code (more on trade-off here). The intermediate steps allowed us to pause, and prepare a reverse dependency (PACTA_analysis/) for the change (in create_interactive_report/).

Even in well structured systems, issues with reverse dependencies occasionally force the kind of intermediate step we did here. But here the interdependencies are all over the place so the issues are more frequent than occasional.

[AlexAxthelm]

I think this case is a good example for git-flow and related workflows. smaller commits can be made into working branches, with the developers actively concerned can really get into the weeds on the internal workings, but then when making the branch from dev to main/master, it gives everyone on the project a chance to weigh in, and make sure that nothing unexpected breaks.

[maurolepore]

I feel the dilemma and welcome this discussion, but I wouldn't worry too much because I think we are in a much much better place than we used to be. By working together we have come a long way.

We seem to already understand the main principles, and I doubt more detailed instructions would be easy to write or cover all possible situations.

The main idea that comes to mind is this: Aim for continuous improvement; move towards your goal as much, as clearly, as safely, and as independently as possible.

When you balance these elements, sometimes you realize you can't reach your goal in a single step. That is the core idea of "refactoring moves".

[maurolepore]

In Google's engineering guide, size translates to time and it recommends to prioritize team speed.

I may move faster today if I push a large PR, but my reviewer will take longer so in the end we get it done faster if I push more but smaller PRs.

[MonikaFu]

my philosophical dilemma is about whether it is indeed faster - if you have all these different PRs related to each other and actually being one thing... And I am not saying that I have the answer - just wondering.

[AlexAxthelm]

Speed should not be your primary goal in development. It's nice to have, to be sure, but functionality and maintainability should be the primary goals. First and foremost, the system needs to work correctly, and it needs to be able to continue working well into the future. Smaller PRs encourage both of those goals, by helping ensure that each individual section is functional, because they each get closer attention when viewed in isolation. There is still the concern of them working together as an integrated system, but that point can also be addressed through testing and respecting the contracts (implicit or explicit) that the system has in place.

[cjyetman]

I also think an interesting process is to imagine if you were working on a project completely alone, but still using Git. Now imagine if you come back to this PR/merge a month from now, or a few months from now for whatever reason. Whatever stress, angst, difficulty you have then, multiply that by 10 and that will be a good estimate of what anyone else will experience if they have to look at it.

[MonikaFu]

I understand - my point is exactly trying to figure out if it would be easier or more difficult to see 3 PRs that constitute 1 feature or 1 PR related to that one feature. I get that less lines of code changed are easier to check - but if you need to look into 3 different PRs to understand what happened for that feature to be implemented - I'm not sure if that is easier.

[cjyetman]

for sure... my suggestion was not that the obvious answer is "smaller".... quite the contrary, I 100% agree that in some cases having 3 related PRs wrapped into one might be much easier to interpret (by your future self, or by someone else)... in which case the thought process I'm suggesting here might tell you to put the PRs together and go "bigger".

[maurolepore]

Just came across (again) with this wonderful article that answers (1) What is small, and (2) When large is okay.

https://google.github.io/eng-practices/review/developer/small-cls.html

[maurolepore]

I came across this super relevant article: https://www.netlify.com/blog/2020/03/31/how-to-scope-down-prs/