201–210 of 302 posts
One thing I like about it is that reviewers can suggest changes and you can accept them inline. Makes it really easy to deal with nits.
Earlier quoted context omitted.
Yeah, it seems like Gerrit with lots of Google-specific stuff. Not surprised. Used Gerrit for 12 years, and loved it.
Gerrit is pretty crap compared to critique. It has a workflow that works for Android, but critique is really much better.
Graphite[0] is also similar in that space(code review platform built on GitHub), the CLI could use some work but combined with the web UI it scratches that same itch that Critique did for me
One thing I like about it is that reviewers can suggest changes and you can accept them inline. Makes it really easy to deal with nits.
You can do that in Github, but for some reason a lot of reviewers are not familiar or bother doing that. Fixing nits that way or giving a suggestion improves turnaround speed greatly and builds a relationship between reviewer and proposer and the final product/commit(s).
One thing I like about it is that reviewers can suggest changes and you can accept them inline. Makes it really easy to deal with nits.
You can do that in Github, but for some reason a lot of reviewers are not familiar or bother doing that. Fixing nits that way or giving a suggestion improves turnaround speed greatly and builds a relationship between reviewer and proposer and the final product/commit(s).
I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.
The non-readability reviewers were usually on your team, so there was social pressure in both directions. You wanted to learn and conform to the team's norms, and the reviewer couldn't be a total jerk about their comments. Everyone was generally on their best behavior.
I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.
I don't know if it's the tool honestly. I've recently switched teams and I'm in the process of getting java readability. The whole experience so far has been much worse than getting my C++ readability. I even get "nit" comments on the code I haven't changed. I have had multiple comments where reviewer basically "preferred" one style over another. It's been still mostly helpful, but I've had my share of frustration. C…
However, I'd say that getting nit comments on code that surrounds your changes, but that you didn't change, is still fair game. It's part of the leave your campsite cleaner than you found it. It depends on the culture though - if it's suggested in the manner of "since you're here, here's an opportunity for how to improve this area of the code", that's better then acting like you made a mistake in failing to change it.
Earlier quoted context omitted.
They are not suggesting hazing. I don't doubt that you experienced hazing. Being vigilant with new hires to assure assignment on code quality and design is not hazing though. If someone has legitimate concerns about the design decisions made, then they should voice them. However if they are refusing to adhere to guidelines, simply because they dislike the approach then that's being overly problematic.
It’s literally the definition of hazing. But instead of being asked to jump in a pool, naked, while snowing, you are asked to build things a new hire has no business building. Then nit-picked for not knowing things. Literally set up for failure. A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of…
If your team is insulting you or saying that you're a bad engineer based on code review comments, that's a bad team. That doesn't mean that ensuring new team members learn the team's style and patterns is hazing.
Earlier quoted context omitted.
I definitely didn’t mean it like hazing. I don’t see what’s so wrong about saying “you used inheritance for this relationship, but we have a pattern of keeping classes like this separate, since this system tends to change frequently. Please organize this like xyz module instead.” Just a random example of something that a new person might do who is unfamiliar with xyz module and the complications there. I agree that i…
My response would be a simple "Why does code changing frequently prevent inheritance from being used?" if I got a comment like that. Granted, I don't like inheritance, so I have nearly 1000 arguments on reasons not to use it that has nothing to do with code changing frequently, so ... this is probably a bad example for me, personally. But seriously, I'd ask why, and why again. I'm very much against cargo culting, and…
Change is bad unless it's great. Being able to look around a codebase and know how things work because similar coding styles and patterns are consistently followed is a huge productivity boost (this is also what commenters complaining about the idea of readability elsewhere in this thread are missing). Something must be 10x better to compensate for diverging from those patterns.
What you write is not YOUR code. It is your TEAM'S code, and the good of the team is far more important than what you personally like.
Earlier quoted context omitted.
Anything subjective that doesn't fix an identifiable execution problem must be explicitly labeled as a suggestion . You don't get first choice over the code because you are asked to be the reviewer. You are there to (1) catch mistakes and (2) teach the other coder if they appear to not know something useful that you do know. If you have a preference about a simple stylistic matter that is not covered by your style gu…
IMO you should never provide feedback that can be implemented as an automated check. If you don't like deeply nested control flow, then you should catch that with static analysis. If you need code coverage, you should require it for merging. Implement your check and provide a new PR to fix your nitpicks, or shut up. The goal is to put 100% of the focus on correctness.
I’m always surprised to see the totality of support for this workflow. My biggest gripes were: - Owners - Readability review (or really the 18month queue to get Java/python readability) Although seemingly innocuous, this made maintaining internal libraries very challenging. There was no way to update every call site of your library in an efficient way. Tools like Rosie were added on top so that you could shard your P…
Ownership is the easiest issue to solve here. Beyond a few dozen clients, you're going to run into the inability to run tests fast enough, and beyond a few hundred clients, you won't be able to sync fast enough, even if you skip all presubmits (there will, in expectation, always be a merge conflict in some file). 3-stage migrations/rosie are necessary at that point anyway. Owners is only the problem at like, 5-10 clients, and when I made changes at that scale, messaging owners was usually fast and effective.