3 ms·

So much dev time goes up in smoke because of pedantic and overzealous linters, formatters, etc. in PR checks and CI pipelines. Your whole PR gets held up becau

by Ameo 4y ago

So much dev time goes up in smoke because of pedantic and overzealous linters, formatters, etc. in PR checks and CI pipelines. Your whole PR gets held up because of a style nit like an unused import or trailing whitespace. That forces you to pull up the PR in Github, dig into the CI failure, check out your branch again, make the fix, commit, push, and wait for CI all over again. If your PR was previously approved, then you have to bug the reviewer to re-approve since the review gets dismissed when you push new commits. I know this isn't exactly what this article was talking about, but it's such a big frustration to me. I'm all for leaving warnings like this on and cleaning them up over time, but yeah I strongly believe that turning all lints and checks and warnings all the way up and then gating PRs behind them is a recipe for huge developer frustration and loss of velocity.

mhh__ 4y ago
A lot of this is a symptom of CIs being deliberately dumb to avoid being wrong, and our workflows being shit. Fuck the CI, the velocity is quite bad just from the programmers machine. One small thing that arcanist (random example) does that raw git does not is making it very "automatic" to make a really simple change to some code. Even theoretically very trivial things like guessing which files you meant to add to the commit can be a real time saver. In fact, contrary to what I just wrote, it's not even entirely about time - a lot of this is really about mental context switching. (Also it's not universally true that you need re-approval)
jacobsenscott 4y ago
We have a fairly good middle ground - our linters only alert on changed lines. They run locally with a commit hook, so you can fix them before they hit the build. A clean linter run isn't required to merge, we trust devs to review the warnings and decide if they should fix them. Also you can turn off the github "feature" that dismisses the review when you push a new change. Trust devs to only make trivial changes after the review - or to re-request the review themselves if they make bigger changes.
Centigonal 4y ago
Sounds like your team may benefit from pre-commit hooks! Our team has an onboarding script we have every new dev run that (among other things) installs pre-commit. Our main repo includes hooks for style, max file size, etc. If there's a style nit, git will yell at you and not accept the commit. If possible, it'll actually correct the style issue (e.g. blackify a python file for you) and you can just stage the updated file. We still have CI/CD, but that catches other errors, not linting issues. This method has worked pretty well for us so far.
- pornel 4y ago
  These hooks must be really high quality — fast and reliable. Otherwise they will get in the way, slowing down development or even breaking git. List of problems include: • the hook runs tools directly on the working directory, and not on the stage/index, so it lints against a mixed version of the code, and not what is actually being committed. • the hook tries to fix the problem above, but doesn't understand submodules and screws them up • a new version of a linter adds a new warning that fires on existing code, and now nobody can commit anything without disabling the hook. • hook expects the tools to be in PATH that isn't set as expected when git is used via IDE or some other GUI tool and so on.

by Ameo 4y ago

mhh__ 4y ago

A lot of this is a symptom of CIs being deliberately dumb to avoid being wrong, and our workflows being shit. Fuck the CI, the velocity is quite bad just from the programmers machine. One small thing that arcanist (random example) does that raw git does not is making it very "automatic" to make a really simple change to some code. Even theoretically very trivial things like guessing which files you meant to add to the commit can be a real time saver. In fact, contrary to what I just wrote, it's not even entirely about time - a lot of this is really about mental context switching. (Also it's not universally true that you need re-approval)

jacobsenscott 4y ago

We have a fairly good middle ground - our linters only alert on changed lines. They run locally with a commit hook, so you can fix them before they hit the build. A clean linter run isn't required to merge, we trust devs to review the warnings and decide if they should fix them. Also you can turn off the github "feature" that dismisses the review when you push a new change. Trust devs to only make trivial changes after the review - or to re-request the review themselves if they make bigger changes.

Centigonal 4y ago

Sounds like your team may benefit from pre-commit hooks! Our team has an onboarding script we have every new dev run that (among other things) installs pre-commit. Our main repo includes hooks for style, max file size, etc. If there's a style nit, git will yell at you and not accept the commit. If possible, it'll actually correct the style issue (e.g. blackify a python file for you) and you can just stage the updated file. We still have CI/CD, but that catches other errors, not linting issues. This method has worked pretty well for us so far.

pornel 4y ago
These hooks must be really high quality — fast and reliable. Otherwise they will get in the way, slowing down development or even breaking git. List of problems include: • the hook runs tools directly on the working directory, and not on the stage/index, so it lints against a mixed version of the code, and not what is actually being committed. • the hook tries to fix the problem above, but doesn't understand submodules and screws them up • a new version of a linter adds a new warning that fires on existing code, and now nobody can commit anything without disabling the hook. • hook expects the tools to be in PATH that isn't set as expected when git is used via IDE or some other GUI tool and so on.