Should I be enforcing lint rules on feature branches?

Question

I am working on a project and I typically setup precompile git hooks for things like linting and testing. I recently had some consultants we use complain about requiring lint on every commit. Their response is they commit as they go and they want to be able to check in half finished code on a feature branch. My personal opinion is that it is too risky to have bad code in ANY branch. Their question to me was how do we handle half finished changes when someone needs to leave, which to me is a great question. So it is mine here....

Should I require Linting and testing on EVERY commit or should I limit it to say pushes to master?

Answer 1

I would 100% save this for pushes to a shared branch, e.g. Main or Development.

In my experience, pushing half-finished code (to ones OWN branch) is a very common thing to do for situations such as finishing for the day, switching machines, remote working or even just pushing so someone else can pick it up, so your implementation seems quite limiting.

Why don't you have a linter with a shared configuration as part of your dev dependencies so it's always being enforced, and save the testing and other processes for merging to a shared branch?

Better still, have you tried setting up a DevOps pipeline to handle all of this automagically?

Answer 2

I don't understand the argument against applying linting and static analysis to every commit on a shared branch. Since shared branches typically live in a remote environment (often GitHub, GitLab, or Bitbucket), this translates into executing linting and static analysis around a push. It's one thing to check-in (or push) work that's unfinished in the sense that the unit of work's requirements or acceptance criteria haven't been met, but I don't see the value in checking in a change that results in a broken build, especially to a shared environment where other people may use the work.

First, I'd suggest that waiting for commit time is too late, at least for linting. In many cases, the linter can be integrated with the editor or IDE, giving real-time feedback on many potential problems as the developer is typing. For anything that isn't automatically correctable, this will make it readily apparent to the developer that something needs to be corrected. Perhaps it's OK to correct it later and make a commit into a feature branch with some unresolved linter warnings, but the number can be reduced with the editor integration.

The idea of checking in "half-finished" code is somewhat worrying, as well. What, exactly, does "half-finished" mean? If the code is introducing something that will break the build, like compiler errors or syntax errors that will cause unhandled exceptions at runtime (in interpreted languages), I can't think of a situation where that would be acceptable. Even if the change is incomplete, the application should not be in a broken state. The use of keystone interfaces and feature flags can also help to hide unfinished work if some changes need to be integrated upstream.

Answer 3

It goes back to the perennial question about what a developer's preferred Git workflow should be and what a commit should be:

• Is a commit an atomic unit of work - something that you can examine in isolation, read its commit message to understand what it does by itself, do a git bisect to it, etc.?
• Or is a commit some arbitrary checkpoint in someone's personal and idiosyncratic workflow, and the larger branch is the unit of work, and individual commits may or may not matter (and there's a decent chance you'll squash or rebase them away when you're done)?

If a commit is a unit of work, then it should get whatever static analysis checks, linting, and automated tests are practical. If the branch is the unit of work, there's not necessarily much need to check individual commits, and individual commits may be broken.

Personally, I really like the commit-as-a-unit-of-work approach. I find it very valuable to have a more granular history than the branch level, and it's nice for tools like git blame and git bisect. But there are good arguments and strong feelings on both sides.

I also believe that faster feedback is better. (You'll see people express this idea as "shorten feedback loops," "shift left," etc.) If I've made an accidental mistake, I'd rather find out when I commit than when I push or merge.

You can also lint on every commit as a default but allow exceptions; as long as the team is okay with this practice, you can always add a --no-verify to ignore hooks and let you commit anyway if someone needs to commit some work in progress.

Answer 4

I’d say it’s the wrong question. You can, if you wish to do so, make any checks when code is shared . With a decent compiler that works quite well even without an enforced lint. The question then is: At which point is code shared?

I put my code on the server for backup, or to be able to check it out myself from that branch in a different place. It’s not shared until a pull request is merged and should be finished just before the merge.

So you have a conflict where new people commit changes before they are shared. That’s your real problem. If at all possible add checks before a pull request is created.

Answer 5

I would say this might differ depending on the context.

How "big" is a typical feature and a feature branch? Assume each feature, and changes in feature branches tend to be relatively small, so that merges into the main branch occur frequently. In that case I would be tempted to consider a feature branch almost a "personal" branch, belonging to whoever is working on it. As such, I think it should be up the that developer to decide whether or not it should be tested on every commit.

If a feature branch is a larger beast, and several developers are working on it over time, then I would agree that linting / testing on each commit would be very useful. In this case however, each dev may well have his or her own personal branch in addition to the common feature branch, and I would not want to enforce testing / linting on those.

Why not? Because on principle I would not want to force any restrictions on developers that early in the coding process. Later, when merging to the feature branch? Sure. When merging to the main branch? Definitely! Still I don't see why a dev shouldn`t be able to commit whatever he or she wants to his or her own branch.

In my personal experience, I try to avoid committing code that is not ready (i.e. no broken builds, no failing tests, etc), but we all know that in real life, there are some systems that are large, complicated, and sub-optimally designed, and that occasionally need some major refactoring. Sometimes it can be useful to perform such refactoring in more than a single step, and this may result in two or more commits, between which the system is not in a functioning state. I would still like to be able to commit my changes though, and to push them to my personal branch. I would also like for a copy of that branch to reside on the server as a backup, even though I don't expect anyone else to need access to it.

Once all my refactoring is complete and pushed to my branch, I might squash all my commits and commit it to the main branch, some other feature branch. At this point I would be fine with any testing / linting rules that blocked my commit if I'd made a mistake, or if the overall system didn't build. For my own personal branch though, I would prefer to avoid that.

Answer 6

You must make sure everyone in the team understands the value of having a uniform code style. Once they do they will not be able to leave a line “unlinted”. Do not enforce a particular code style though. Intstead, bring the team together and have everyone participate in the decisions.

Nowadays linting can be plugged in to any IDE and executes on the fly while programming. So there is really no reason to leave it for the CI step.

There is some great bibliography about this, Clean Code by Robert Martin the most notorious one. In the book they explain why coming together with a code style for the whole team makes code better, more readable and less prone to errors.

If you are still not convinced, look at big companies: AirBnb has released their own set of guidelines that are a reference for lots of programmers and of course.

Answer 7

"Speaking in my not-so past life as a developer," I definitely agree that you should concern your linting efforts "only to branches, not to leaves."

While a developer is in the process of writing any fairly-involved bit of code, (s)he quite naturally reaches various points where ... well ... "if I screw this next bit up completely, I want to be able to reliably get back to where I started." And they probably also want to be able to "push" those changes ... just in case.

("Yeah. Sometimes this happens ...")

But – until, and unless, those leaf- branches are actually "merged into some trunk," they're not really intended to be "real." Therefore, you don't need to scrutinize them.

Answer 8

So you're trying to dictate what code your colleagues push to their personal branches? Then next step would be to control what they are committing to their local branches. Followed by controlling what they pushing to their own personal repositories on GitHub.

When you say something's risky - you need to also mention what the risks are. I'm pretty sure that if you think about it more you won't find any. These personal branches are often just a way to stash unfinished changes. After person pushes something, the next commit often rewrites the previous one. So what was the purpose of checking commits that don't exist anymore?

Answer 9

My personal opinion is that it is too risky to have bad code in ANY branch.

While the root of that opinion is understandable, this is not a workable standard.

When you hold every commit to this standard, it starts being under tension from both sides. On the one end, you want to commit frequently for data loss prevention and manageable incremental changes, but on the other hand you need to perfectly clean your code before you can even commit, which means either having to constantly refactor everything even when still not even in a final or workable state, or putting off your commits until the implementation is complete and rounded enough to be able to meaningfully refactor it for code quality standards.

What you're asking for is essentially the same as requiring that the floor be clean at the end of every day, even in the middle of a month long renovation project. Or, as another analogy, that the workshop in the back is exactly as clean as the showroom in front. The cost of doing so is disproportionately high compared to the actual benefit gained.

Much like how that floor should only be cleaned at the end of the renovations, not constantly during; or when the product moves from the workshop to the showroom; the code quality should only be judged when trying to merge/pull into a non-feature branch (master, dev, ...).

If you think about it, we use feature branches specifically so that works in progress are in their own little bubble, so that they can be half finished without affecting the source code that is expected to be good and ready at any time (master, dev, ...). When you guard code quality at the gate (i.e. merge/pull), there is no gained benefit from the code quality inside the feature branch while in progress.