4 ms·

Rules I would have everyone follow if I was a dictator: 1. 1 PR, 1 commit. 2. 1 PR cannot have more than 50 lines of product code added. Any number of lines c

Rules I would have everyone follow if I was a dictator: 1. 1 PR, 1 commit. 2. 1 PR cannot have more than 50 lines of product code added. Any number of lines can be removed. You can have up to 100 lines of test code. 3. Every PR should include a set of tests for the added/changed functionality. They must pass. 4. Git merge is forbidden. Everyone must rebase. 5. Every PR goes into master. Nobody can push to master. You can create as many feature branches you like, but definition of done is that your code is available on master. 6. Identify relevant existing test cases and make sure they are passing. 7. Master must always be in a state that it can be deployed instantly.

lamontcg 4y ago
> 1 PR cannot have more than 50 lines of product code added. Any number of lines can be removed. You can have up to 100 lines of test code. Hard disagree. Landing a single class/module to implement new functionality may take more than 50 line of code and 100 lines of test, even though conceptually the additional functionality is simple and it amounts to mostly boilerplate and looks like every other class that implements the pattern. Some large plumbing refactorings or CI refactorings are difficult to do in small chunks. I'm perfectly fine with a PR that replaces one internal API with another one and then has hundreds of small copypasta fixes littered around the codebase to swap out the APIs. Anyone should be able to read that without cognitive overload. Tests are also commonly over 100 lines of code since they tend to be repetitive by nature, and I'd like to not see those artificially broken up into smaller PRs since the most important question I've usually got is if all the cases are covered or not. Breaking 300 lines of tests up into 3 different PRs to satisfy some kind of line-length metric means now the most important question I have needs looking at all 3 simultaneously and that is deeply counterproductive and totally useless. And small code changes with lots of comprehensive tests which fill out cross products of different API usages are great and should be encouraged. And while I don't personally like the dozens-of-tiny-atomic-commits approach, I deeply don't care if other people do it or not. I've never found it useful to read their PRs, or even after the fact, but I won't stand in the way of them doing that if it is what gives them enjoyment (OTOH, I never do that).
- AtlasBarfed 4y ago
  Maybe a good rule for projects in maintenance mode. Large scale refactors? Well, that's impossible under these rules, well unless you go full LISP on what a line does.
  - lamontcg 4y ago
    Mostly what I've done is maintenance mode, that's where if you try to touch something it is often very complicated in secondary effects and fixes, and requires lots of tests. And additions of functionality which are bolt-on will come with 10 years of accumulated requirements around the shape of any new code and the tests.
qz_kb 4y ago
What kind of simple features are you writing that 50 lines seems like enough to do anything useful??
- perryizgr8 4y ago
  You can write any feature with this constraint, if you break it down into smaller units. In fact, it produces better quality code, more readable, fewer bugs. If you find yourself unable to subdivide the task into small units, maybe your system isn't well architected. In that case this won't work for you.

lamontcg 4y ago

> 1 PR cannot have more than 50 lines of product code added. Any number of lines can be removed. You can have up to 100 lines of test code. Hard disagree. Landing a single class/module to implement new functionality may take more than 50 line of code and 100 lines of test, even though conceptually the additional functionality is simple and it amounts to mostly boilerplate and looks like every other class that implements the pattern. Some large plumbing refactorings or CI refactorings are difficult to do in small chunks. I'm perfectly fine with a PR that replaces one internal API with another one and then has hundreds of small copypasta fixes littered around the codebase to swap out the APIs. Anyone should be able to read that without cognitive overload. Tests are also commonly over 100 lines of code since they tend to be repetitive by nature, and I'd like to not see those artificially broken up into smaller PRs since the most important question I've usually got is if all the cases are covered or not. Breaking 300 lines of tests up into 3 different PRs to satisfy some kind of line-length metric means now the most important question I have needs looking at all 3 simultaneously and that is deeply counterproductive and totally useless. And small code changes with lots of comprehensive tests which fill out cross products of different API usages are great and should be encouraged. And while I don't personally like the dozens-of-tiny-atomic-commits approach, I deeply don't care if other people do it or not. I've never found it useful to read their PRs, or even after the fact, but I won't stand in the way of them doing that if it is what gives them enjoyment (OTOH, I never do that).

AtlasBarfed 4y ago
Maybe a good rule for projects in maintenance mode. Large scale refactors? Well, that's impossible under these rules, well unless you go full LISP on what a line does.
- lamontcg 4y ago
  Mostly what I've done is maintenance mode, that's where if you try to touch something it is often very complicated in secondary effects and fixes, and requires lots of tests. And additions of functionality which are bolt-on will come with 10 years of accumulated requirements around the shape of any new code and the tests.

qz_kb 4y ago

What kind of simple features are you writing that 50 lines seems like enough to do anything useful??

perryizgr8 4y ago
You can write any feature with this constraint, if you break it down into smaller units. In fact, it produces better quality code, more readable, fewer bugs. If you find yourself unable to subdivide the task into small units, maybe your system isn't well architected. In that case this won't work for you.