I kind of with the sentiment. Review pre merge though, but only block the merge if there are serious faults. Otherwise, merge the code and have the author address issues after the merge. Get the value to production

[–] Mossheart@lemmy.ca 17 points 2 years ago (2 children)

have the author address issues after the merge.

Hahahahahahaha. Sorry, you've merged, next ticket, PM needs shiny results for execs this QBR!

This is how bug backlogs grow.

[–] SmoothLiquidation@lemmy.world 4 points 2 years ago

This exactly. By the time they notice a problem you are three tickets down and on to the next sprint.

[–] Deifyed@lemmy.ml 3 points 2 years ago

Yeah, I see your point. Maybe my employers are different, it's never been an issue explaining why the ticket isn't closed just because the PR is merged

[–] Doveux@pawb.social 3 points 2 years ago

I'm with you. I've worked on a few teams, one of the first was a company where two teams were contributing code changes to the same product. The other team "owned" it and as a result it took ages, sometimes months, to get code changes merged. It meant more time was spent just rebasing (because merging wasn't "clean") than working on the actual feature.

My current role, we just do TDD, pair programming, and trunk-based development. We have a release process that involves manual testing before live deployment. Features that aren't ready for live are turned off by feature flags. It's quick and efficient.

Fundamentally I think the issue is the right tool for the job. Code doesn't need to be managed the same way in a company as it does in an open-source project.

[–] kautau@lemmy.world 2 points 2 years ago

This only works if the merge is being done to staging builds that are continuously tested by a QA team before they go to production, with carefully planned production milestone releases. I work for an emergency management SaaS company. If we just merged all lightly reviewed code into production without thorough QA testing, there’s the possibility that our software would fail in production. This could cause aircraft in major airports to crash into each other on the runway, or a university to respond poorly to a live shooter situation, or the deletion of customer data about COVID vaccine efforts, etc

[–] jjjalljs@ttrpg.network 1 points 2 years ago (1 children)

This is some poe's law shit. I can't tell if you're serious or just committing to the bit.

[–] Deifyed@lemmy.ml 1 points 2 years ago (1 children)

Sorry about the confusion. It's not sarcasm. I'm just sick and tired of people blocking my PR because of an argument about wether the function should be called X or Y or Z or D

[–] jjjalljs@ttrpg.network 2 points 2 years ago

Ah. Yeah those kind of nitpicks are annoying. We try to specify when comments are blocking or non blocking on reviews.

But I definitely block a lot of reviews over no tests, bad tests, no error handling, failed linting. And the occasional "this doesn't do what the ticket asked for"