> PR approval is too boolean. The PR is approved or it's not approved. Real code review, like real life, lives in the middle
This is have-your-cake-and-eat-it. PR approval is a permission so is a boolean. Of course it is. Either the code can be merged or it can't.
What's being described really here is just something to make you feel slightly better about yourself whilst approving code you hate ("we should revisit this..."). Just open a new ticket.
I was in camp 'boolean', but I think this has convinced me. I always had a problem that there were developers who didn't really understand the code, but would click 'approve' anyhow because they didn't see any problems in the parts they understood.
This meant that they were completely unable to actually 'approve' a review, but were only able to reject it. They were juniors, so they'd eventually get to that point, but by then, everyone would be used to just ignoring their approvals.
The nuance is comments on the PR itself, rather than the state of the approval, which is binary (or ternary, if you want to count leaving it in an unknown state for extended periods of time).
The PR needs to have someone who knows the whole thing.
Having several people review each separate parts but not understanding the others' can cause interaction bugs. If such bugs cannot happen (say, due to modularity, or type safety guarantees etc), then it won't be the case where you need to have a partial approve.
I am not a fan of partial approve. Either you think the code is approvable, or it isn't.
I mean, that’s fair no? If the UX creates an impasse for the user then this leads to friction in the process. There’s more than one way to address it. One is the user overcomes his own internal dilemma, the other is the UX helps him get there. For example, would be cool if there was a way to do a conditional approval with an issue tied to a stacked PR or something similar (just throwing ideas, point is to surface up the friction as a UX take not a protocol or API issue with git)
ah, thank you. Haven't worked in java for a bit now, but that was the only one I read where I was like "I'm sure we didn't have to avoid this when I worked on java".
The rest were all very familiar. Well, apart from the new stuff. I think most of my code was running in java 6...
But in my case it was the other way around. I work in a Kowloon Walled City of code: dozens of intersecting communities with thousands of informally organized but largely content contributors. It looks like chaos, but it works ok.
Code formatting really did feel like a new neighbor declaring "you know what this place needs, better-marked bus lanes!" as though that would help them see the sky from the bottom of an ally or fix the underlying sanitation issues. As you might imagine, the efforts didn't get far and mostly annoyed people.
But as the GP said, it all depends on the culture. If you pick up and move to Singapore you'd damn well better keep your car washed and your code style black.
I mean fired and resigned when it became clear you'd be fired are the same thing really.
We're not actually entitled to know the exact details of someone's job ending. They worked there. Now they don't. That much is the bit we're entitled to.
For public misconduct like this, we should get to know if he was fired (or asked to resign) as opposed to his making the independent decision to find work elsewhere or retire or whatever. We should get to know if he left because the company wanted him gone or because he wanted to be gone.
This has been done very professionally. They pulled the article. They handled the personnel matter. They didn't try to pretend it hasn't happened.
Why are people here acting like retracting an article is an attempt to hide something. They literally replaced the whole text with a note from the editor saying "this article was bad".
This is have-your-cake-and-eat-it. PR approval is a permission so is a boolean. Of course it is. Either the code can be merged or it can't.
What's being described really here is just something to make you feel slightly better about yourself whilst approving code you hate ("we should revisit this..."). Just open a new ticket.
reply