71–80 of 80 posts
Earlier quoted context omitted.
Wouldn’t most people just restate the ticket? Or do you mean, they write about the actually implementation. Eg, “this fixes the race condition by removing the timeout and waiting until the initialization task finishes (line #55) before instantiating the chart widget”
I long for tickets that descriptive
Earlier quoted context omitted.
At work, I've been writing a word document to explain every part of a program I've written and how each element interacts with the rest. It's been very helpful to find things that don't make as much sense as I thought they did when I wrote them, and revealing old code that is no longer used anywhere in the application. And I've caught a few places where outdated .txt / .md / inline-doxygen documentation lies, too. Wr…
But wouldn't this summary document be better as a form of comment in the source code rather than a separate document that will become out of date the next time someone else makes changes?
Code reviews are not very good: - Most style issues should be caught by automated tools - Most functional issues should be caught by tests (written by another person preferably) - To bring someone up to speed on your chosen style, pair programming is much faster than code reviews - To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster Not sure why we have falle…
Nope. People have biases, and if you are teaching coding practices via pair programming you will just propagate those biases. By having a new team member receive feedback from multiple members, these biases are more likely to average out. For style specifically, it's also wise to get reviews from other teams for new people.
I much prefer to go in and check people's small commits to master than look at a huge PR before it gets merged in, but it requires you (and the whole team) to be really diligent and do it on a daily basis, and when it comes to big features, you lose a lot of the context. OTOH, the amount of times I've been presented with a huge PR of an entire feature where the author begins by saying "sorry for the huge PR" and my predictable reply "I'm having trouble getting the whole picture here, but LGTM?" are countless, so I'm not sure that's better either.
Not sure how to reconcile the two models into something great.
As for the style of code review, I ask the team and adjust accordingly. I can be "big picture guy", or I can do that and simultaneously be "Mr. Nitpick" if people so wish (some people actually do, me included).
I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to approve a PR, you should summarize everything the PR does in the approval comment. Responses like ":+1:" or "LGTM" were not allowed. When I would read a PR intending to understand and then summarize, the errors or omissions (eventually) started to just jump out at…
Interesting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?
The old me would read it, and write something like "adds S3 bucket and IAM role".
The newer me would write it as:
1. Creates S3 bucket in account .
2. Creates IAM role , with an attached policy granting read-only access to S3 bucket .
It seems like a small change, but when I'm actually planning to type out those details - I'm more likely to notice typos in the name/account/role/etc, or just outright errors. One of the mental shifts from my old reviews to newer ones is that I open the PR assuming I don't know what it does yet. So I have to read it for myself and interpret it, then summarize it for someone else.
> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! Our CI automatically deploys a "review app" when someone creates a pull request. This simplifies code review, QA and demoing!
It sounds cool that you create on-demand review app. In a small scale it works. In a larger scale development is absolute impossible due to resource constraints to create on-demand environment for review app. For instance, in a warehouse automation infrastructure that has 30 devs, working on 8 feature simulataneously, it is next to impossible to create review app with the resources they need.
Using serverless hosting, the hosting bill is < $10/mo.
Code reviews are not very good: - Most style issues should be caught by automated tools - Most functional issues should be caught by tests (written by another person preferably) - To bring someone up to speed on your chosen style, pair programming is much faster than code reviews - To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster Not sure why we have falle…
I do a lot of Typescript coding. Using Prettier/eslint rules, a host of (e.g. hundreds) of common stylistic and functional issues are caught in CI/CD automatically.
Better yet, I've made a `npm run fix` command that will fix 99% of them instantly.
If your tooling can automatically bring the code into compliance, then folks won't push back and will be highly likely to comply :)
Pair programming is sooo much better than PR’s. I really loathe reviewing pull requests, no matter if you do it absolutely correct, and don’t miss anything, it’s extremely wasteful of the time of both the coder and the reviewer, even if there are no changes that need to be made. There is the context switch for the reviewer, ditto for the coder, waiting for feedback. Ugh. Just have two people do the work together, and…
If I'm doing a PR (e.g. for a new dev), and it's becoming a real bloodbath, I stop doing the PR and do a 1:1 pair programming session w/ the dev. This helps save face and keeps the advice private and inculcation.
Don't let PR's turn into public floggings.
Earlier quoted context omitted.
Interesting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?
Here's a contrived example... Let's say a PR arrived that appeared to create a new S3 bucket and a new IAM role with an attached policy that grants read-only permissions to that bucket. The old me would read it, and write something like "adds S3 bucket and IAM role". The newer me would write it as: 1. Creates S3 bucket in account . 2. Creates IAM role , with an attached policy granting read-only access to S3 bucket .…
Earlier quoted context omitted.
How big are these changes? That sounds horrendously slow. If you don't trust your tests, write better tests. If you don't trust your migrations, write better tests.
In probably most cases the person who does the code changes does write tests for it as well. So as a reviewer without understanding what the code and the tests do, how do you want to assess if the tests actually do something useful?
Because otherwise that's how you get write-only code. Things like code style and documentation can make a big difference, and review time is when you can help enforce them and build team culture and knowledge.