Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Code review can be better

tigerbeetle.com

Post reply on HN
Print view View original

211–220 of 253 posts

Re: Code review can be better

#211

Post by gen220 »

mytailorisrich wrote: post #174
ncruces wrote: post #172

Earlier quoted context omitted.

“What you'll do next and in what way” is often an important tool to put the small changes into context. Stacked pull requests can be an important tool to enable “frequent, small changes” IMO. Sure, I can use a single pull request and a branch on top of that, but then it's harder for others to leave notes on the future, WIP, steps. A common situation is that during code review I create a few alternative WIP changes to…

From a continuous integration perspective my understanding is that stacked pulled requests do not make change more frequent if we define a "change" as being committed on the master branch. They only split the feature branch into smaller chunks. On the other hand, I do take your point about context over a number of consecutive changes. But, to me, "creating a few alternative WIP changes to communicate to a reviewer" i…

FWIW, stacking is "just a tool" that you can use to make whatever sort of workflow you want. I agree creating alt PRs isn't a high-value usage of the tools.

The //actually better// workflows stacking enables are the same sort of workflows that `git add -p`, `git commit --fixup` and `git rebase` enable, just at a higher level of abstraction (PRs vs commits).

You can "merge as a stack" as you imply, but you can also merge in sub-chunks, or make a base 2-3 PRs in a stack that 4 other stacks build on top of. It allows you to confidently author the N+1th piece of work that you'd normally "defer" doing until after everything up to N has been reviewed.

An example: I add a feature flag, implement a divergent behavior behind a feature flag gate, delete the feature flag and remove the old behavior. I can do this in one "stack", in which I deploy the first two today and the last one next week.

I don't have to "come back" to this part of the codebase a week from now to implement removing the flag, I can just merge the last PR that I wrote while I had full context on this corner.

In theory you can do all of this stuff with vanilla git and GitHub. In non-stacking orgs, I'd regularly be the only person doing this, because I was the only one comfortable enough with git (and stacking) for it to not be toooo big a burden to my workflow. Graphite (and other stacking tools) make this workflow more accessible and intuitive to people, which is a big net win for reviewers imo.

Re: Code review can be better

#212

Post by gen220 »

globular-toast wrote: post #110
tomasreimers wrote: post #16

Just taking a step back, it is SO COOL to me to be reading about stacked pull requests on HN. When we started graphite.dev years ago that was a workflow most developers had never heard of unless they had previously been at FB / Google. Fun to see how fast code review can change over 3-4yrs :)

As someone who already breaks tasks into atomic (or near atomic) pieces and always has done, is this just submitting a PR for each commit as you go? How does it work for breaking changes? Requires use of feature flags?

Sort of, yeah! It lends itself well to a 1 PR = 1 commit philosophy. Every PR has to pass CI to be mergeable. If you want to make a CI-breaking change, putting that behind a feature flag is one valid strategy.

I'd recommend giving it a try to see what it's like. The `gt`/onboarding tour is pretty edifying and brief.

It's likely that you'll find that `gt` is "enabling" workflows that you've already found efficient solutions for, because it's essentially an opinionated and productive subset of git+github. But it comes with some guardrails and bells and whistles that makes it both (1) easier for devs who are new to trunk-based dev to grok and (2) easier for seasoned devs to do essentially the same work they were already doing with fewer clicks and less `git`-fu.

Re: Code review can be better

#213

Post by ivanjermakov »

spike021 wrote: post #177
ivanjermakov wrote: post #153

Earlier quoted context omitted.

By read-only I meant that you can't fully interact with the code: run/debug it, use intellisense, etc.

Can't you just check out the branch of the repo locally into your ide? i'm still confused what limitation you are talking about.

Described well in the post. This way you have to switch between ide and web diff viewer, redundant and not convenient.

Re: Code review can be better

#214

Post by sdeframond »

benji-york wrote: post #186
sdeframond wrote: post #141

Earlier quoted context omitted.

I share your feelings. Regarding design reviews, we used to have them at my current job. However we stopped doing both formal design documents and design reviews in favor of prototyping and iterative design. The issue with the design phase is that we often failed to account for some important details. We spent considerable time discussing things and, when implementing, realized that we omitted some important detail o…

> The ideal setup is to put 5 people in the same room with the PO and close to a few key users. (I suspect you are aware, but just in case this is new to you.) This is essentially the core of Extreme Programming.

What team size does XP recommends, if any ?

It seems like the standard around me is between 8 to 12 people. This is too many in my opinion.

I believe this is because management is unknowingly aiming for the biggest team the does not completely halts instead of seeking a team that delivers the most bang for the buck.

Re: Code review can be better

#215

Post by jmholla »

wolvesechoes wrote: post #127
jmholla wrote: post #116

Earlier quoted context omitted.

I also read this series of blog posts recently where the author, Hillel Wayne, talked to several "traditional" engineers that had made the switch to software. He came to a similar conclusion and while I was previously on the fence of how much of what software developers do could be considered engineering, it convinced me that software engineer is a valid title and that what we do is engineering. First post here: http…

Personally I don't need to talk with "traditional" engineers to have an opinion there, as I am mechanical engineer that currently deals mostly with software, but still in the context of "traditional" engineering (models and simulation, controls design). Definitely making software can be engineering , most of the time it is not, not because of the nature of software, but the characteristics of the industry and culture…

I think it's still be worth your time to check out the articles as they talk about other fields of engineering and how they compare to software AND mechanical.

Re: Code review can be better

#216

Post by stonemetal12 »

hgomersall wrote: post #156
wolvesechoes wrote: post #127

Earlier quoted context omitted.

Personally I don't need to talk with "traditional" engineers to have an opinion there, as I am mechanical engineer that currently deals mostly with software, but still in the context of "traditional" engineering (models and simulation, controls design). Definitely making software can be engineering , most of the time it is not, not because of the nature of software, but the characteristics of the industry and culture…

Engineering is just about wielding tools to solve problems. You don't need to use formal methods to do engineering in general. Sometimes they're useful; sometimes they're required; often they just get in the way. In the context of software vs other sub-disciplines, the big difference is in the cost of iterating and validating. A bridge has very high iteration cost (generally, it must be right first time) and validati…

>Engineering is just about wielding tools to solve problems. You don't need to use formal methods to do engineering in general.

Way to general to be useful. By that definition the store clerk is an engineer (tool cash register, problem solved my lack of gummy bears), janitors swinging a mops, or automotive techs changing oil.

Engineering is applied science.

Re: Code review can be better

#217

Post by benji-york »

sdeframond wrote: post #214
benji-york wrote: post #186

Earlier quoted context omitted.

> The ideal setup is to put 5 people in the same room with the PO and close to a few key users. (I suspect you are aware, but just in case this is new to you.) This is essentially the core of Extreme Programming.

What team size does XP recommends, if any ? It seems like the standard around me is between 8 to 12 people. This is too many in my opinion. I believe this is because management is unknowingly aiming for the biggest team the does not completely halts instead of seeking a team that delivers the most bang for the buck.

I don't know of a canonical XP team size, but "small" is the normal recommendation. 8-12 would seem larger than small to me.

Personally, 8 is the largest I would have a team. At that point you should consider breaking it into two teams of 4 (even if those teams periodically recombine from the original set of 8 people).

Re: Code review can be better

#218

Post by stonemetal12 »

mytailorisrich wrote: post #137
motorest wrote: post #136

Earlier quoted context omitted.

> You need a high level design up-front but it should not be cast in stone. Yes, you need a design that precedes code. > Writing code and iterating is how you learn and get to a good, working design. You are confusing waterfall-y "big design upfront" with having a design. It isn't. This isn't even the case in hard engineering fields such as aerospace where prototypes are used to iterate over design. In software engin…

> You are confusing waterfall-y "big design upfront" with having a design. I do not and I have explained it. > In software engineering fields you start with a design and you implement it And part of my previous comment is that this "waterfall-y" approach in which you design first and implement second does not work and has never worked. > you do not need to pay the cost of a big design upfront Exactly, and not only th…

>How many experiments, prototyopes, iterations go into building a car or a rocket? Many. Engineers do not come up with the final design up front.

No where did anyone claim you need the full final design up front. For cars\rockets how many of those experiments, prototypes, and iterations had designs? All of them. You never see a mechanical engineer walk out to the shop and just start hammering on a pile of slop until it sort of looks like a car.

>The difference it is that this is expensive while in software we can iterate much more, much quicker, and for free to get to the final product.

If you have no design to meet how do you judge the output of an iteration or know you have arrived at the final product?

Re: Code review can be better

#219

Post by sdeframond »

benji-york wrote: post #217
sdeframond wrote: post #214

Earlier quoted context omitted.

What team size does XP recommends, if any ? It seems like the standard around me is between 8 to 12 people. This is too many in my opinion. I believe this is because management is unknowingly aiming for the biggest team the does not completely halts instead of seeking a team that delivers the most bang for the buck.

I don't know of a canonical XP team size, but "small" is the normal recommendation. 8-12 would seem larger than small to me. Personally, 8 is the largest I would have a team. At that point you should consider breaking it into two teams of 4 (even if those teams periodically recombine from the original set of 8 people).

I find it non trivial to split a team in two independent teams.

If the two teams have to coordinate a lot and work on the same code base, is there still two teams?

To be independent, they would need to work on functionnaly different parts of the project. Not all projects have several independent parts, feature-wise.

What do I miss?

Re: Code review can be better

#220

Post by epolanski »

pjmlp wrote: post #173
epolanski wrote: post #87

Earlier quoted context omitted.

> and that isn't something I ever encountered in the wild (in any formal sense) Because in the software engineering world there is very little engineering involved. That being said, I also think that the industry is unwilling to accept the slowliness of the proper engineering process for various reasons, including non criticality of most software and the possibility to amend bugs and errors on the fly. Other engineer…

There is plenty on large scale enterprise projects, but than whole that stuff is looked down by "real developers". Also in many countries, to one call themselves Software Engineer, they actually have to hold a proper degree, from a certified university or professional college, validated by the countrie's engineering order. Because naturally 5 year (or 3 per country) degree in Software Engineering is the same a six we…

In Italy a degree isn't enough, you need to take an exam and be certified.
Post reply on HN