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

Ask HN: I'm tired of intense code review cycles

news.ycombinator.com

Post reply on HN
Print view View original

41–50 of 55 posts

Re: Ask HN: I'm tired of intense code review cycles

#41

Post by bracketslash »

pydry wrote: post #36
siquick wrote: post #10

I had a Lead like this. It go to the point where I would spend a significant portion of my working time in existential dread about submitting a PR because I knew they would just rip it to pieces. The worst part was small styling issues that would have been picked up by any linter - but the lead refused any requests for the team to use linting because “we have never used it and everything works fine” I lasted 4 months…

Your lead was a junior. One of the reasons I always fought to have black from almost the day it existed was because (I thought) it would put an end to this type of petty human linting.

what is “black” in this context?

Re: Ask HN: I'm tired of intense code review cycles

#42

Post by xmcqdpt2 »

bracketslash wrote: post #41
pydry wrote: post #36

Earlier quoted context omitted.

Your lead was a junior. One of the reasons I always fought to have black from almost the day it existed was because (I thought) it would put an end to this type of petty human linting.

what is “black” in this context?

The python formatter, I guess.

https://github.com/psf/black

Re: Ask HN: I'm tired of intense code review cycles

#43

Post by amhoab »

bracketslash wrote: post #41
pydry wrote: post #36

Earlier quoted context omitted.

Your lead was a junior. One of the reasons I always fought to have black from almost the day it existed was because (I thought) it would put an end to this type of petty human linting.

what is “black” in this context?

https://github.com/psf/black

Re: Ask HN: I'm tired of intense code review cycles

#45

Post by tharkun__ »

krimbus wrote: post #38
gruez wrote: post #16

>- The focus of reviews is not on correctness but stuff like naming, docstrings, and design. I agree that focusing on naming/docstrings doesn't make much sense, but design feels like it's fair game. What type of "design" stuff is showing up in your reviews?

Indeed! Design suggestions are fair game, but they are quite rare nowadays and their quality has been dwindling. The last one I got felt too arbitrary: I decided to improve the behavior of an unused feature from one of our core libraries to unblock a colleague working in the application. The change was straightforward and it took me 1hr to get it into a PR. The feedback was that we should keep the old behavior availa…

I read all of the comments here with much interest. Thanks for putting in this specific example.

I would love to have a chat with your architect. If you (or he) gave me a PR with

    foo(Timeout connection)
I would ask him to improve the naming. That thing is not a connection. In the places this variable will be used it makes no sense to use a connection. Take this example usage, potentially way down in the implementation of foo. Let's say an imaginary send function that takes the data and a timeout).

    send(data, connection)
If I read this code I will think that it takes data and a connection. But it doesn't. It takes data and a timeout. I have to know or look up the type of connection to understand what is happening.

Instead your original

    foo(Timeout connection_timeout)
Will make absolute perfect sense without knowing or having to look up the type. The following code can be read without having to stop and cross reference information. What you see is what you get. Less cognitive load, which is awesome. You put in thought to make awesome readable code once and every subsequent reader for all eternity can enjoy being able to just read and immediately understand what your code does and what everything is.

    send(data, connection_timeout)

Re: Ask HN: I'm tired of intense code review cycles

#46

Post by peakaboo »

pydry wrote: post #36
siquick wrote: post #10

I had a Lead like this. It go to the point where I would spend a significant portion of my working time in existential dread about submitting a PR because I knew they would just rip it to pieces. The worst part was small styling issues that would have been picked up by any linter - but the lead refused any requests for the team to use linting because “we have never used it and everything works fine” I lasted 4 months…

Your lead was a junior. One of the reasons I always fought to have black from almost the day it existed was because (I thought) it would put an end to this type of petty human linting.

I have to disagree with the view that not using black means the lead is not senior.

Being senior means you understand the tradeoffs and you make your own decision.

If you ask a senior why he choose not to use kubernetes, you will get a bunch of real reasons. If you ask a junior the same, he won't have a good answer because he lacks experience with the pros and cons.

Re: Ask HN: I'm tired of intense code review cycles

#48

Post by nucleogenesis »

peakaboo wrote: post #46
pydry wrote: post #36

Earlier quoted context omitted.

Your lead was a junior. One of the reasons I always fought to have black from almost the day it existed was because (I thought) it would put an end to this type of petty human linting.

I have to disagree with the view that not using black means the lead is not senior. Being senior means you understand the tradeoffs and you make your own decision. If you ask a senior why he choose not to use kubernetes, you will get a bunch of real reasons. If you ask a junior the same, he won't have a good answer because he lacks experience with the pros and cons.

Right and “we don’t need to use black because we haven’t yet and things work fine” is a bad response that dismisses the real needs and concerns of others on the team when they’re saying “things aren’t fine we want computers to handle the nitpicks from here”. I wouldn’t say it indicates junior tech skills but it’s a sign of junior management skills.

Re: Ask HN: I'm tired of intense code review cycles

#49

Post by tossaway9000 »

siquick wrote: post #10

I had a Lead like this. It go to the point where I would spend a significant portion of my working time in existential dread about submitting a PR because I knew they would just rip it to pieces. The worst part was small styling issues that would have been picked up by any linter - but the lead refused any requests for the team to use linting because “we have never used it and everything works fine” I lasted 4 months…

I don't know why someone would want to waste time arguing about code style.

Standardizing on things like `cargo fmt`, `go fmt`, and `terraform fmt` remove a ton of nitpicking out the gate. The javascript world can't seem to make up their mind though (jslint is rarely used these days, jshint died, I think eslint is the thing now?)

Re: Ask HN: I'm tired of intense code review cycles

#50

Post by pydry »

peakaboo wrote: post #46
pydry wrote: post #36

Earlier quoted context omitted.

Your lead was a junior. One of the reasons I always fought to have black from almost the day it existed was because (I thought) it would put an end to this type of petty human linting.

I have to disagree with the view that not using black means the lead is not senior. Being senior means you understand the tradeoffs and you make your own decision. If you ask a senior why he choose not to use kubernetes, you will get a bunch of real reasons. If you ask a junior the same, he won't have a good answer because he lacks experience with the pros and cons.

>I have to disagree with the view that not using black means the lead is not senior.

I disagree with this straw man too.

It's the code review focus on small almost irrelevant stylistic details that signals a lack of experience.

Post reply on HN