October 1, 2025 at 12:00 am
Comments posted to this topic are about the item PRs Are Like Trouble Tickets
October 1, 2025 at 6:33 am
There is a conflict between people doing the reviews and people requiring the reviews. They interrupt the flow.
I would prefer a PR that is small and focused, but that requires people who are prepared to do a lot of PRs. The scarcity of reviewers means that I have to submit PRs that are bigger and broader, which take longer to review.
There is the question of what we want PRs to cover. Linting, formatting, testing, code quality, and security scanning should be part of the CI/CD workflow and also available on our workstations. That way, you are not doing a PR on basic code hygiene; you are doing a PR on the important stuff.
I've found that Co-Pilot makes useful observations on a PR.
I describe the intent and decisions for my PR using markdown. My team find this approach valuable as it gives them the context for the PR. A little bit of time upfront greases the wheels for them and, in doing so, makes reviewing a PR less onerous.
I am beginning to understand the viewpoint of pair programming aficionados. It isn't two people doing the work of one. It's a real-time PR that is a preventative rather than a retrospective, and prevention is better than a cure, even when that cure isn't refused as "risk-accepted".
October 1, 2025 at 8:52 am
When you need to change an old procedure for the first time and decide to format it or make some other cosmetic changes, to this stuff first and check it in.
Particularly formating tends to produce tons of changed lines and it is hard to find the real code changes in the lot of formating stuff.
God is real, unless declared integer.
October 1, 2025 at 12:07 pm
...
There is the question of what we want PRs to cover. Linting, formatting, testing, code quality, and security scanning should be part of the CI/CD workflow and also available on our workstations. That way, you are not doing a PR on basic code hygiene; you are doing a PR on the important stuff.
...
Agreed, PR review should be focused on material changes and the impact, nothing else.
This also usually means people need to be trained (And reminded ) of what a good PR looks like. And a bad one
October 1, 2025 at 2:11 pm
I think a big thing is not being too rigid within a system. If we can use common sense, we don't have backlogs of pointless reviews and we don't have people pushing out dangerous changes without reviews. There has to be balance, and it takes the right people/culture to have that balance.
Be still, and know that I am God - Psalm 46:10
October 1, 2025 at 2:17 pm
I read today's SSC newsletter with great interest. Following good software development practice has been an interest of mine for a long time. But, due to a lack of being around teams that practiced it, I've no practical experience. The US State government department I work for is the largest IT organization I've ever worked in, but there's never been any good practice here. I think in large part that's because the vast majority of applications we've written have been small, not requiring more than one developer on it.
But that is changing. We've working in a large project which is taking longer than a year to complete. There's several developers working on it. However, we've still got problems. One contractor who's working on it has a nasty habit of doing everything out of sight for weeks on end. Then, in one massive commit, he'll add a whole new, completely working application to the repo, create a PR and assign the PR to the most junior developer on the team to review. Of course the poor newbie isn't going to know how to review a whole application in a PR, so he'll do just the basics (open it in Visual Studio, then see if it builds). We've told this contractor to break up commits into smaller chucks, but he couldn't care less. In fact, this whole PR process is nothing more than a PITA to him.
OK, this guy's approach to PR is the worst, but there are others, who don't take it as far, but still greatly resent the discipline of doing good team development. I wish we could bring in some consultancy to teach us how to do it better. But I also know that won't happen.
Kindest Regards, Rod Connect with me on LinkedIn.
October 1, 2025 at 3:47 pm
When you need to change an old procedure for the first time and decide to format it or make some other cosmetic changes, to this stuff first and check it in. Particularly formating tends to produce tons of changed lines and it is hard to find the real code changes in the lot of formating stuff.
When you need to change an old procedure for the first time and decide to format it or make some other cosmetic changes, to this stuff first and check it in.
Particularly formating tends to produce tons of changed lines and it is hard to find the real code changes in the lot of formating stuff.
A good reason to have a standard format the people use, or that gets applied with a Githook
October 2, 2025 at 2:08 pm
Ah, so "PR" is an acronym for Pull Request.
Most of the time when I'm doing a code review for a stored procedure, the context is that it was actually deployed to production a week ago, and I get a trouble ticket asking me to investigate why the database has been spiking CPU or timing out.
Each of us has our own observable universe, but at least where I work, 90% of the time the problem is not a poorly written SQL query - but rather something like:
All this points to some additional development or configuration work on the part of the DBA required to support the new data access paradigm, whether it be creating / altering an index or maybe replicating / ETL-ing a table so it's available locally.
"Do not seek to follow in the footsteps of the wise. Instead, seek what they sought." - Matsuo Basho
October 14, 2025 at 5:58 pm
PRs are a good way to try and catch issues early. Running all deployed code through a PR doesn't necessarily prevent what you've listed, but it does give someone a chance to catch things.
Or, if they've been warned about these items being an issue, look for this before deployment.
Viewing 9 posts - 1 through 9 (of 9 total)
You must be logged in to reply to this topic. Login to reply