The biggest grip I have with Github is the app is painfully slow. And by slow, I mean browser tab might freeze level slow. Shockingly, the best code review tool I've ever used was Azure DevOps.
> The biggest grip I have with Github is the app is painfully slow. And by slow, I mean browser tab might freeze level slow. Javascript at scale combined with teams that have to move fast and ship features is a recipe for this. At least it's not Atlassian.
Code review can be better
151–160 of 253 posts
Re: Code review can be better
#152WORKFLOW
Every repository is personal and reviewer merges, kernel style. Merging is taking ownership: the reviewer merges into their own tree when they are happy and not before. By implication there is always one primary code reviewer, there is never a situation where someone chooses three reviewers and they all wait for someone else to do the work. The primary reviewer are on the hook for the deliverable as much as the reviewee is.
There is no web based review tool. Git is managed by a server configured with Gitolite. Everyone gets their own git repository under their own name, into which they clone the product repository. Everyone can push into everyone else's repos, but only to branches matching /rr/{username}/something and this is how you open a pull request. Hydraulic is an IntelliJ shop and the JetBrains git UI is really good, so it's easy to browse open RRs (review requests) and check them out locally.
Reviewing means pushing changes onto the rr branch. Either the reviewer makes the change directly (much faster than nitpicky comment roundtrips), or they add a //FIXME comment that IntelliJ is configured to render in lurid yellow and purple for visibility. It's up to the reviewee to clear all the FIXMEs before a change will be merged. Because IntelliJ is very good at refactoring, what you find is that reviewers are willing to make much bigger improvements to a change than you'd normally get via web based review discussions. All the benefits the article discusses are there except 100x because IntelliJ is so good at static analysis. A lot of bugs that sneak past regular code review are caught this way because reviewers can see live static analysis results.
Sometimes during a review you want to ask questions. 90% of the time, this is because the code isn't well documented enough and the solution is to put the question in a //FIXME that's cleared by adding more comments. Sometimes that would be inappropriate because the conversation would have no value to others, and it can be resolved via chat.
Both reviewee and reviewer are expected to properly squash and rebase things. It's usually easier to let commits pile up during the review so both sides have state on the changes, and the reviewer then squashes code review commits into the work before merging. To keep this easy most review requests should turn into one or two commits at most. There should not be cases where people are submitting an RR with 25 "WIP" commits that are all tangled up. So it does require discipline, but this isn't much different to normal development.
RATIONALE
1. Conventional code review can be an exhausting experience, especially for junior developers who make more mistakes. Every piece of work comes back with dozens of nitpicky comments that don't seem important and which is a lot of drudge work to apply. It leads to frustration, burnout and interpersonal conflicts. Reviewees may not understand what is being asked of them, resulting in wasted time. So, latency is often much lower if the reviewer just makes the changes directly in their IDE and pushes. People can then study the commits and learn from them.
2. Conventional projects can struggle to scale up because the codebase becomes a commons. Like in a communist state things degrade and litter piles up, because nobody is fully responsible. Junior developers or devs under time pressure quickly work out who will give them the easiest code review experience and send all the reviews to them. CODEOWNERS are the next step, but it's rare that the structure of your source tree matches the hierarchy of technical management in your organization so this can be a bad fit. Instead of improving widely shared code people end up copy/pasting it to avoid bringing in more mandatory reviewers. It's also easy for important but rarely changed directories to be left out, resulting in changes to core code slowing down because it'd require the founder of the company to approve a trivial refactoring PR.
FINDINGS
Well, it worked well for me at small scale (decent sized codebase but a small team). I never scaled it up to a big team although it was inspired by problems seen managing a big team.
Because most questions are answered by improving code comments rather than replying in a web UI the answers can help LLMs. LLMs work really well in my codebase and I think it's partly due to the plentiful documentation.
Sometimes the lack of a web UI for browsing code was an issue. I experimented with using IntelliJ link format, but of course not everyone wants to use IntelliJ. I could have set up a web UI over git just for source browsing, without the full GitHub experience, but in the end never bothered.
Gitolite is a very UNIXy set of Perl scripts. You need a gray beard to use it well. I thought about SaaSifying this workflow but it never seemed worth it.
Re: Code review can be better
#153I find the idea of using git for code reviews directly quite compelling. Working with the change locally as you were the one who made it is very convenient, considering the clunky read-only web UI. I didn't get why stick with the requirement that review is a single commit? To keep git-review implementation simple? I wonder if approach where every reviewer commits their comments/fixes to the PR branch directly would w…
is github's PR considered read-only? i've had team members edit a correction as a "suggestion" comment and i can approve it to be added as a commit on my branch.
Re: Code review can be better
#154Re: Code review can be better
#155Earlier 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…
Re: Code review can be better
#156Earlier 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…
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 validation is proven over decades. Software has very low iteration cost, so it makes much more sense to do that over lots of upfront design. Validation of software can also generally be implemented through software tools, since it's comparatively easy to simulate the running environment of the software.
Other disciplines like electronics live a little closer to a bridge, but it's still relatively cheap to iterate, so you tend to plan interim design iterations to prove out various aspects.
Re: Code review can be better
#157Here's an alternative I've wondered about: Instead of one person writing code, and another reviewing it - instead you have one person write the first pass and then have another person adjust it and merge it in. And vice-versa; the roles rotate. Anyone tried something like this? How did it go?
Great idea, if you're fine with development time to take twice as long.
Re: Code review can be better
#158Just 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 :)
So I’m really hoping something like Graphite becomes open-source, or integrated into GitHub.
Re: Code review can be better
#159The biggest grip I have with Github is the app is painfully slow. And by slow, I mean browser tab might freeze level slow. Shockingly, the best code review tool I've ever used was Azure DevOps.
Re: Code review can be better
#160Earlier 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…
No, the big difference is that in the Engineering disciplines, engineers are responsible end-to-end for the consequences of their work. Incompetence or unethical engineers can and regularly do lose their ability to continue engineering.
It's very rare that software developers have any of the rigour or responsibilities of engineers, and it shows in the willingness of developers to write and deploy software which has real-world costs. If developers really were engineers, they would be responsible for those downstream costs.