Hacker News new | ask | show | jobs
by davedx 2350 days ago
Senior here, 20 years experience, started out with C++ projects where the only quality gate was the build server failing and currently working on a project where commits fail because of lint rules including things like imports being alphabetized.

IME the best approach is a "balanced" one. Let devs push their branches from local to the repo without passing tests or lint rules. (e.g. git push --no-verify). Try and keep your tests fast so devs want to run them locally. (Carrot, not stick). But prevent a merge request from being merged to your trunk branch until everything is green.

Personally I find "linters" horrible. They're like someone with OCD leaning over your shoulder while you code. I think VS Code actually runs a LOT slower when the linter is integrated, which can do more harm than good. How crazy is it that TypeScript can do all its advanced type checking faster than the linter?

5 comments

Yes, completely agree. Also alphabetizing includes in C++ is just wrong. The more basic file should be below the less basic file. This is because the less basic file may need to include the more basic file itself and the compiler no longer complains about the missing include if it appears above it.

OCD & often wrong pretty much summarizes linters for me.

I think we're on the same page here.

The only caveat I'd add is that it depends on the project and the team how many gates you want to add. A couple of super-senior devs building a small blog aren't going to need many gates. A team with 50 people, half of them juniors, working on a financial app already in production - they're going to need a lot of gates.

As with all these things, ask the team if they're adding value. If they are great, and is there anything else that would help? If they're not, and especially if they're unnecessarily slowing things down, then don't do them. Don't use a code style checker because somebody in a meetup said you should.

In my experience, any project with excessive automation and such rules quickly turns into a game of "find how to pass the checks" and you spend the majority of the time fighting the bureaucracy rather than doing useful work.
If your devs don't care about the quality of code they deliver, they'll find a way to deliver whatever crap they've built - even if it takes longer to find a workaround than just write some good code.

CI/CD should not be used to force developers to do things they don't want to do, but to automate some easily repeatable tasks they would still do anyway.

> But prevent a merge request from being merged to your trunk branch until everything is green.

You want to go even further: only merge feature branches were you know that they won't break master after the merge.

Some feature branches might pass tests by themselves, but something has changed with master since they were branched off.

Bors (https://bors.tech/) helps with that.

Yep, forcing MR's to be rebased to the last master/develop commit helps a lot here. IIRC this is a feature you can enable in Gitlab. Much less chance your MR will break master/develop if your work is rebased on the latest commit.
Bors doesn't force you to rebase. Forcing the rebase is super annoying in projects with more than a few developers (I've worked on one that did).

What bors does is essentially the same thing, but automated: after all the tests have run on your MR branch, and people have approved your changes, bors does a speculative merge into master, and runs the tests on that merge commit.

If the tests pass, bors declares that very same commit to be the new master.

The effect is the same as the Gitlab option you mention, but the busy work is done by the bot.

I assume these tools are configurable, so devs could agree what's the balance. We usually start with defaults (assuming somebody put effort into choosing the defaults) and customize as we go.

Pushing without verify and fix everything at the end is worse than not fixing at all imho. After all fixes you basically have a lightly refactored code that has been less tested.