5 ms·

Regarding PRs. I have a an opinion which has become very unpopular since the rise of github. The way github does PRs is great for open source projects where yo

Regarding PRs. I have a an opinion which has become very unpopular since the rise of github. The way github does PRs is great for open source projects where you have submissions from a very diverse set of developers. You need to evaluate the whole PR as whole and accept it or reject it. But it is not ideal where you have a small team working on single product. The PR review becomes a gate and it has been my experience as a developer that you spend a LOT of time trying to get your PR reviewed. My preferred approach is to do smaller and more frequent commits that are merged directly to the main branch. Developers learn to break a problem into very small changes which is a skill by itself. It is the responsibility of a lead or Sr engineer to check the commits as they come so they can provide feedback as soon as possible. This was the spirit of the original continuous integration ideas back in the day.

huflungdung 7mo ago
[dead]
gdulli 7mo ago
Modern software engineering culture is a treadmill ever in search of the next best practice that must be applied to a field made up of bespoke scenarios.
steveklabnik 7mo ago
Stacked diffs let you not have to evaluate the whole PR as a whole, and encourages smaller and more frequent commits. Basically, I agree with you that large PRs are a problem, I just don't think it means you need to throw pre-land code review out the window.
- sharts 7mo ago
  Agree too bad stacked PRs aren’t really native to GH and you probably need to go with not so standard tools to manage them
  - steveklabnik 7mo ago
    Yes, it is unfortunate. I did link an announcement below that some form of this feature is coming, we'll see what it actually looks like when it goes public.
gib444 7mo ago
What does the full story look like in your preferred approach? Regarding releasing for example
htobc 7mo ago
That's pretty much how Google does dev, though not everyone there is consistent about small CLs or the <24 hour SLO for code review. But yeah, if the team lead is aware of what everyone is working on, and prioritizes fast CLs review, huge amounts of friction and slowdown are removed from the process
- dhosek 7mo ago
  What I’ve seen is things like I ask a question about a piece of code during a PR, the author changes that code and my question vanishes into the ether with no indication (unless it’s lost in the noise of email notifications) that the code was changed and my question is no longer relevant (and if there was, perhaps an answer, the answer is also lost).
  - steveklabnik 7mo ago
    Yes. This specific aspect of GitHub is the reason why many teams don't want you to modify commits, but instead, add more commits. Which then also leads to squash-merging branches. Other systems, like Gerrit, handle this much better!
deleted 7mo ago
[deleted]
bardackx 7mo ago
this is how we work using graphite
smcameron 7mo ago
PRs can contain multiple commits. You need something like stgit to make it easy to make a bunch of small commits that appear to be the work of an omniscient genius who knew exactly what they were doing. Try using stgit for awhile, and you'll wonder how you ever lived without it.
- deleted 7mo ago
  [deleted]
demorro 7mo ago
> you spend a LOT of time trying to get your PR reviewed. I've fixed this issue in several teams I've joined. Maybe some people won't understand this, as I didn't until I observed it multiple times over. There are teams out there, perhaps the majority, that simply leave the decision as to when a PR will be reviewed nebulous, and at the discretion of team members. There is no formal obligation to do them in a timely manner, and there are no consequences if they are not done. The solve to this is obvious and easy: - Automatically assign specific people to PRs. No general team assignment. The submitter can add specific people in addition to PR's if they need domain experts, but the normal case is random assignment. - Require PR's to be done within 24 working hours. If you cannot do this for whatever reason, you must communicate this to the team. - There are consequences if you violate this policy. The last one is the hard part for cowardly teams and cowardly managers. You do have to stay on top of it initially, and even when people have accepted this and gotten used to it, you can't forget about it because people will drift. This isn't to say I'm against the direct integration model you propose, I can also see the appeal, it's just that problems with PR flows are mostly about cowardly management, not anything much to do with the actual process.
- pavel_lishin 7mo ago
  At a previous job, it was the on-call engineer's duty to review PRs, as one of their responsibilities. A PR is just another interrupt, right? We also paired this with giving the on call engineer near total freedom with what to do during their shifts, similar to 20% time (which was about the same percentage; 5 team members, weekly on call rotations.) They chose which tickets to pick up from the backlog, which also helped keep up with maintenance and taking care of bugs and issues that otherwise wouldn't get prioritized.

huflungdung 7mo ago

[dead]

gdulli 7mo ago

Modern software engineering culture is a treadmill ever in search of the next best practice that must be applied to a field made up of bespoke scenarios.

steveklabnik 7mo ago

Stacked diffs let you not have to evaluate the whole PR as a whole, and encourages smaller and more frequent commits. Basically, I agree with you that large PRs are a problem, I just don't think it means you need to throw pre-land code review out the window.

sharts 7mo ago
Agree too bad stacked PRs aren’t really native to GH and you probably need to go with not so standard tools to manage them
- steveklabnik 7mo ago
  Yes, it is unfortunate. I did link an announcement below that some form of this feature is coming, we'll see what it actually looks like when it goes public.

gib444 7mo ago

What does the full story look like in your preferred approach? Regarding releasing for example

htobc 7mo ago

That's pretty much how Google does dev, though not everyone there is consistent about small CLs or the <24 hour SLO for code review. But yeah, if the team lead is aware of what everyone is working on, and prioritizes fast CLs review, huge amounts of friction and slowdown are removed from the process

dhosek 7mo ago
What I’ve seen is things like I ask a question about a piece of code during a PR, the author changes that code and my question vanishes into the ether with no indication (unless it’s lost in the noise of email notifications) that the code was changed and my question is no longer relevant (and if there was, perhaps an answer, the answer is also lost).
- steveklabnik 7mo ago
  Yes. This specific aspect of GitHub is the reason why many teams don't want you to modify commits, but instead, add more commits. Which then also leads to squash-merging branches. Other systems, like Gerrit, handle this much better!

deleted 7mo ago

[deleted]

bardackx 7mo ago

this is how we work using graphite

smcameron 7mo ago

PRs can contain multiple commits. You need something like stgit to make it easy to make a bunch of small commits that appear to be the work of an omniscient genius who knew exactly what they were doing. Try using stgit for awhile, and you'll wonder how you ever lived without it.

deleted 7mo ago
[deleted]

demorro 7mo ago

> you spend a LOT of time trying to get your PR reviewed. I've fixed this issue in several teams I've joined. Maybe some people won't understand this, as I didn't until I observed it multiple times over. There are teams out there, perhaps the majority, that simply leave the decision as to when a PR will be reviewed nebulous, and at the discretion of team members. There is no formal obligation to do them in a timely manner, and there are no consequences if they are not done. The solve to this is obvious and easy: - Automatically assign specific people to PRs. No general team assignment. The submitter can add specific people in addition to PR's if they need domain experts, but the normal case is random assignment. - Require PR's to be done within 24 working hours. If you cannot do this for whatever reason, you must communicate this to the team. - There are consequences if you violate this policy. The last one is the hard part for cowardly teams and cowardly managers. You do have to stay on top of it initially, and even when people have accepted this and gotten used to it, you can't forget about it because people will drift. This isn't to say I'm against the direct integration model you propose, I can also see the appeal, it's just that problems with PR flows are mostly about cowardly management, not anything much to do with the actual process.

pavel_lishin 7mo ago
At a previous job, it was the on-call engineer's duty to review PRs, as one of their responsibilities. A PR is just another interrupt, right? We also paired this with giving the on call engineer near total freedom with what to do during their shifts, similar to 20% time (which was about the same percentage; 5 team members, weekly on call rotations.) They chose which tickets to pick up from the backlog, which also helped keep up with maintenance and taking care of bugs and issues that otherwise wouldn't get prioritized.