But PR discussions about lintable style issues always surprise me. The ideal solution is to add a rule in the linter. But when the team won't agree on the rule, and is open to multiple styles, the author should decide, simple! Had a team mate recently who'd block PRs over T[] vs Array<T>, forcing people to stick with Array<T> for simple types like number[] even though TypeScript's own docs and tooling push T[].
Linters often don’t provide the constraints you desire. While they’re a great (arguably essential) tool, they often are not sufficient.
When a project has well-established patterns, part of the job is to just follow the pattern, whether you like it or not, until you find a reason to 1) change the pattern everywhere, or; 2) make an exception to the pattern with intention.
It's quite simple to write a rule, e.g. for the example I shared https://typescript-eslint.io/rules/array-type. But I've never seen any reputable project arguing against T[] for simple types. In my opinion, that says a lot about the reviewer, not the contributor. If a linter rule doesn't exist, you should argue against the idiomatic convention.
Yeah, I think all style comments should be handled by either a linter/formatter or be written in a style guide. Everything else is up to personal preference.
Then, when a style comment comes up in a PR, the answer is "Oh, do you think we should add that to our style guide? If so, let's discuss that in slack. Until then, though, that's not blocking."
This is exactly the pedantry I try to avoid in my teams.
The point of a review shouldn’t be to make sure it’s exactly how the reviewer would do it. Is it safe? Does the author understand the area? Is there evidence the code has been exercised? Does it follow the major conventions?
Beyond that, I try very hard to build a culture of approvals vs nitpicking, and let the linters enforce the rest.
Yep that's how I do it if I have to deal with stacked PRs. I also just never use rebase once anything has happened in a PR review that incurs historical state, like reviews or other people checking out the branch (that I know of, anyways). I'll rebase while it's local to keep my branch histories tidy, but I'll merge from upstream once shared things are happening. There are a bunch of tools out there for merging/rebasing entire branch stacks, I use https://github.com/dashed/git-chain.
I also branch out, and rebase. Also, keep updating and rebasing until merged. It’s tedious when PR take ages for approval, as I keep creating new branches on top of each other.
So, when I saw this announcement seemed interesting but don’t see the point of it yet.
Can we start appreciating and respecting other people's professional experiences without dismissing and criticising them?
It's well written and brought to light a very interesting subject, e.g. "Marshall’s research showed that just 15-20% of riflemen in active combat positions ever fired their weapons".
To be honest, GitHub actions made a big impact at a time when every other CI framework sucked, really badly. Maybe today, others are much better than they used to be!
But PR discussions about lintable style issues always surprise me. The ideal solution is to add a rule in the linter. But when the team won't agree on the rule, and is open to multiple styles, the author should decide, simple! Had a team mate recently who'd block PRs over T[] vs Array<T>, forcing people to stick with Array<T> for simple types like number[] even though TypeScript's own docs and tooling push T[].
reply