Yeah, 1k is kinda a small limit but I get the logic, you can almost always break changes into smaller increments and not mass merge a mega PR that’s hard to review
for a new feature request? a PR isn’t a commit, it’s a set of commits which would add to the line change amount.
Like even if you spread it out across 20 or 30 commits that’s still going to be the same line count.
I guess you could push not yet functional or used code to lessen the line count change, but that seems in bad taste. I’ve always gone off the working repo should always be in build or clean state and a push or commit shouldn’t break that.
Stupid question, but what happens to a rejected PR? Because features get built for a reason (there’s usually a Ticket/Story for the feature that the PR adds) so do those just get closed? Or does the person have to re-write the code entirely?
I know in my team, the most I could do is tell the coworker to self-review while keeping the PR open until they change some stuff. I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.
If a feature request requires changes of such a magnitude it is important to break them down into smaller chunks that can be reviewed either independently or sequentially.
I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.
Define bad? If the PR contains lots of unnecessary changes such as formatting or renaming simply tell the person to roll them back and come again unless they have very good reason to do so.
If the code quality is bad, well, that’s why you are doing the review. If all that matters was “Does it do what it is supposed to do most of the time?” some simple unit tests would be enough. Reviewing code means making sure it does what is supposed to do and does so in an acceptable manner. Criteria can be amongst others speed, security, ease of use or maintainability.
Insecure handling of inputs? Add sanitisation and resubmit.
Overusage of resource intensive features such as database queries? Group and optimize queries and resubmit.
The codes formatting is not according to the internal style guide? Configure your damn linter and resubmit.
…
If you don’t want to close PRs outright you can request new commits that fix the issues you identified, reevaluate the PR and decide again.
Our PR checks auto reject the PR if it has 1k changes
is it auto reject, or just doesn’t auto approve and leaves it open for manual review
It seems weird that you can’t do a pr at all with 1000 line changes, any moderate size feature addition could hit that mark
Yeah, 1k is kinda a small limit but I get the logic, you can almost always break changes into smaller increments and not mass merge a mega PR that’s hard to review
You know, you could just chunk it up in a way to keep it readable.
for a new feature request? a PR isn’t a commit, it’s a set of commits which would add to the line change amount.
Like even if you spread it out across 20 or 30 commits that’s still going to be the same line count.
I guess you could push not yet functional or used code to lessen the line count change, but that seems in bad taste. I’ve always gone off the working repo should always be in build or clean state and a push or commit shouldn’t break that.
I work in a Scrum team and we implement features iteratively, so we start with the minimal feature, merge it, get feedback, and go on from there.
At work there’s no excuse to keep a feature in a stale branch until you accumulate 1000 lines of code change.
Stupid question, but what happens to a rejected PR? Because features get built for a reason (there’s usually a Ticket/Story for the feature that the PR adds) so do those just get closed? Or does the person have to re-write the code entirely?
I know in my team, the most I could do is tell the coworker to self-review while keeping the PR open until they change some stuff. I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.
If a feature request requires changes of such a magnitude it is important to break them down into smaller chunks that can be reviewed either independently or sequentially.
Define bad? If the PR contains lots of unnecessary changes such as formatting or renaming simply tell the person to roll them back and come again unless they have very good reason to do so.
If the code quality is bad, well, that’s why you are doing the review. If all that matters was “Does it do what it is supposed to do most of the time?” some simple unit tests would be enough. Reviewing code means making sure it does what is supposed to do and does so in an acceptable manner. Criteria can be amongst others speed, security, ease of use or maintainability.
If you don’t want to close PRs outright you can request new commits that fix the issues you identified, reevaluate the PR and decide again.
What if you remove some files? 🙃