201–210 of 431 posts
Jane Street implements an awesome code review system: https://janestreet.com/tech-talks/janestreet-code-review > [...] Telling a Story with Commits [...] > [...] it should take the average reviewer 5-10 minutes [...] Jane Street code review system kinda solves this problem by - making each commit a branch, - stacking branches on top of each other (gracefully handling rebases and everything that comes with it), and -…
Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices. I trust my colleagues to do the same (and they often do). I can't imagine working in an environment where this is a theater.
It sounds like a good job where the most important part is finding other people’s mistakes. Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very…
And that basically describes all of programming: we are building metaphors that will run electricity at a higher or lower voltage, and be translated again into something meaningful to a different human.
In many ways, all we are doing is performing. And that is some of what makes this job challenging: the practices that build software well are all just ways of checking how humans will interact with the ones and zeros we've encoded.
Returning a PR because it doesn't have tests means that code will have automated validation, which is a real change. It also means the code will be written in a testable way: too often we don't realize we wrote code in a way that is hard to test unless we try to write the tests. And on a larger level, it means that this team of engineers will learn and use the practices and tools that lead to testable code and effective tests and more easily-changeable code.
It makes total sense to not keep reading if there aren't tests, because adding the tests can be expected to change the code. But just because that is a performance doesn't mean it doesn't profoundly change the world.
Earlier quoted context omitted.
> Yes, TDD on production code is nice in theory, but it doesnt work in my case... Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided". I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.
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...).
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.
I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…
Story from friend but I can relate: Some teams and code are pretty much unreviewable and the best thing is to add CI for simple tests and lgtm if there is no glaring mistakes. My team only has 3 including the manager, so, eh, each one holds a lot of knowledge that only he or she knows. Documentation? Yeah that’s a good idea, but I don’t have time to read them because “We want to ship as fast as possible”. So I just p…
It takes two seconds to write `EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY=[1] [...] NOT IN EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY` instead of `NOT IN [1]` and then not hate your past self six months from now when you have to figure out why something has been excluded.
If some code can't be understood today, it's not going to be able to be understood when someone comes to modify it. Maybe in your domain all the code is write-once-read-never, but for those of us maintaining enterprise software that isn't an option.
We absolutely expect our reviewers to fully understand the code. If you want a quick shortcut to make that cultural change, have any bugs in code go to the code reviewer instead of the original author. You will find people start taking code reviews a whole lot more seriously.
Yea, this person knows x so blindly approve.
or
Hey jane/john did you remember to check x before this? Yes! Ok, blindly approve.
PRs have no scope and no way to designate scope. What do I want from each reviewer? At some point, the PR becomes superfluous to adequate code review and communication around said review.
Ultimately they can be used to micromanage and gatekeep merges to main. This is the PR at its worst.
Overall I’m not a big fan, it feels like a necessary evil to meet the demands of Big Agile.
I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…
I often do, In a larger PR or in one where it's hard to tell what is being accomplished, like this article articulates the commits can tell a story of the engineers journey to solution. Even if I review a commit that is largely undone by future commits that piece of history is often key to my understanding.
What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist. You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force. Maybe I've only worked at crappy places
A decent programmer can write code they can effectively work with. The really great programmers write code that even interns can effectively work with. The only way to get to that level is to get good at eliciting and taking in feedback from the people we want to be effective with our code.
It doesn't necessarily mean doing exactly what we are told: it means understanding the why of a comment, what underlying flaws or confusions a comment is pointing towards. It means encouraging people to ask questions in code reviews, rather than just leave commands: often a "how does this manage to do X?" comment points to a place where bugs were hiding anyway, even if also it is a chance to share a language feature.
Many engineers work in companies where being a really great programmer doesn't get you any points. Often the only reward for writing code that is easily modified later is the gratitude of a future developer asked to make a change to it years down the line.
But I am that developer often enough that that's still made the journey worth it for me.
What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist. You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force. Maybe I've only worked at crappy places
Allen Iverson got criticized by the media for “letting down his teammates” for skipping a few practices. He famously said “We’re talking about practice, practice! not the game! How is MY going to practice gonna make THEM better?!” He got flack for those comments, but what he said was accurate. If we’re talking about outcomes you’re beholden to the person who is seen as the difference maker, all the teamwork in the wo…
If you aren't making your teammates better, and they aren't making you better, you will never be able to be as good as a team that is greater than the sum of its parts. Individual genius is consistently beat by professional collaboration.