61–70 of 224 posts
Per the article, not the pull requests itself, but the tedious bikeshedding around them. The article's headline is a bit misleading.
That's probably what they mean by "PR Process"?
Earlier quoted context omitted.
I think a good general advice when training somebody is - focus them only on the biggest flaw at the time. That's how I would proceed with a "bad" PR. I would even accepts smaller issues (style) when there is a bigger issue to fix. People might eventually outgrow it.
For me it's the opposite. If I'm making a PR, please point out ALL of the nits you can even think about. My next PR is bound to have at most 25% of that since by now I get more about what the team values and considers important.
The code is the same way and there was a great comment higher up about an ignored lint rule being an indicator of someone thinking about it and making a judgement call that this time the rule shouldn't apply. I have SOME peers where that my reading of that line of code and others where my assumption is the opposite and they were just lazy or didn't know how to do it "the right way". It's the same line of code!
I don't know how tooling will ever replace this part so I think it's important to keep it as the bedrock of any collaboration process.
My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've…
Raising a planning change equivalent to a 3000 LOC PR in a civil engineering firm would get your assignment swiftly handed over to someone more competent.
Earlier quoted context omitted.
Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it
Style also involves things like "renaming variables", to take the article's example, which can't be automated away.
Personally I have a strong distaste for projects that try to use some metric for how long a function should be, e.g. line count or cyclomatic complexity. I'm not sure if this one is better automated, human judgement for what makes sense seems better to me. Sometimes the cleanest and highest performance way to write some code is going to be basically one relatively large function. If there's a function complexity/size…
Linux C style guide has this summarized pretty well: "The maximum length of a function is inversely proportional to the complexity and indentation level of that function. So, if you have a conceptually simple function that is just one long (but simple) case-statement, where you have to do lots of small things for a lot of different cases, it’s OK to have a longer function. However, if you have a complex function, and…
My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've…
Even with the best of intentions, it's easier to miss things in longer PRs, plus it gives more time for others if they want to have a look. It's not bulletproof of course, but at least it makes sure the basic functionality is working.
When it comes to code reviews, the return on investment faces the Law Of Diminishing Returns. While many of the comments made in code reviews might be interesting, they are not so interesting that they pay for themselves. If you put some dollar value on the time invested, you'll find that the vast majority of this process is simply burning money. And not only money, but also, as this article says, morale. A curious f…