3 ms·
It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when th
by nox100 3y ago
It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted.
In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allow minor edits but it's strict enough that I've rarely seen it pass an edit. I assume there must have been some incident that triggered this.
- lokar 3y agoIME this is always driven by lazy compliance people pushing the most straight forward way to satisfy some audit. Plenty of places subject to strict audit rules manage to make small post review edits work.
- dkersten 3y agoI read it as it’s up to the person who opened the PR whether to apply the nits, or whether to ignore them and use the approval to merge. This is how our codebase works: you’re free to ignore minor suggestions, but if you do edit the code, reviews are dismissed and need to be provided again. Basically, an unaddressed nit doesn’t block merging. But any code changes will require fresh reviews. If the change is very minor, it’s very quick for someone who has already approved to check the last commit diff and reapprove.
- yjftsjthsd-h 3y agoI think it's legitimate to force a re-review for any code change, given that programming is one of those charming areas where a single character can completely change the meaning of something. That said, if you can read the commits individually then final review for something where minor changes were added should be utterly trivial.
- _ea1k 3y agoYes, it is pretty common to require re-review if it is changed again. But approval with comments shifts control back to the submitter. They then have choices: - Modify it and accept that this will trigger re-review - Merge as-is... maybe they don't even agree with the nit - Merge as-is and followup with a new MR under the same JIRA. This sometimes makes sense if it gets them into an important preprod environment, but they are really still working on the issue. - Merge as-is and follow up with a JIRA on the backlog. Maybe they realized that the "nit" is really a bigger issue and should be addressed separately. Regardless, there's value in not blocking the merge for non-functional issues.
- lazyasciiart 3y agoI got a merge blocked last week with the comment 'I think this work is more related to "TFS X: prepare for Y" than it is "TFS Y".' (I eventually realized they wanted me to change the commit message). I hate this fucking team.