The Lounge is rated Safe For Work. If you're about to post something inappropriate for a shared office environment, then don't post it. No ads, no abuse, and no programming questions. Trolling, (political, climate, religious or whatever) will result in your account being removed.
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request...
Would be happy to know other people's opinion!
As a side note, there are many things that I have no clue when reviewing a pull request (like the business process behind) but there is also the fact that we have a developer here who is the best at writing horrible code, and just cant seem to understand the difference between a dictionary and a list.... I mean I just give up with that guy.
I cant complain too much though, he might be the reason why I got a juicy contracting job here, I sometimes think!
The only reason we do everything via pull-request is that this enables us to keep the master from direct changes (in case that the change breaks everything apart)...
So you can not commit directly to the master, only via pull-request, however you can approve yourself... (and the pull-request automatically triggers a build, as a condition to the merge)
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
It's just another tool that can either be used to great effect or as unnecessary clutter. Depends on how the source control is managed.
I worked on a project where pull requests were used for a couple purposes:
1) to ensure merges into critical branches like main-development, production, and releases are explicitly done (not accidental),
2) as a form of code review where a pull request had to be accepted by at least one peer (you couldn't ok your own PR) after filling out a basic checklist (new tests pass, target branch tests still pass, merge is a fast-forward, and a couple others),
and 3) guarantee that git best practices are followed to avoid elephanting the history of the important stuff like ensuring you've merged main-dev into your branch and confirmed nothing broke before fast-forward merging back into main-dev. Fixing a merge problem on an important, shared branch, especially if it isn't caught until a couple merges later, is a nightmare.
It also serves as a discussion/checklist history so if anything does go catastrophically wrong, you not only know who requested the PR but who approved it and can educate them on what went wrong so it doesn't happen again.
You don't really get any of that with plain merges and I'd rather deal with PRs than paperwork. That's my 2 cents. Also I agree with everything Kornfeld said
1.) developer, well, develops
2.) developer tests
4.) PR approved, merge to master
5.) QA tests, UAT by client if needed.
It has been my experience that inflated egos do not jive well with code reviews(PR Reviews). To be honest, it took some time for me to get past my ego, in order to let someone else, whom I may not like, look at my code, and critique it, in the betterment of the product, not my ego.
I thought the same, but working on lqarge projects with 120+ people working on it 24/7 I appreciated the better traceability Pull Request give, because there are less of them.
Uusually we have tons of commits, hundreds of pushes and dozens of pull requests. In the PR we can / have to attach the code static analysis, link the issues related (if any) and have it approved by one or more people.
The integrator does not have to enter knee deep in our commits, he just takes the pull request, merges it and sends to validation. This allows for easier tracing of "approved, definitive changes" and easier regression tracking.
On smaller projects it may be too much, but you never knwo when a project blooms (or explodes).
GCS d--(d+) s-/++ a C++++ U+++ P- L+@ E-- W++ N+ o+ K- w+++ O? M-- V? PS+ PE- Y+ PGP t+ 5? X R+++ tv-- b+(+++) DI+++ D++ G e++ h--- r+++ y+++* Weapons extension: ma- k++ F+2 X
The problem with not putting everything as a PR is how to define it, where to draw the line. You can't say "if it's only one change" or "only one line" don't bother with a PR as that one line\change could have a devastating impact. If you say "use common sense" then one man's common sense if another man's introduction of major bugs.
1. there are other developers to review the code
2. said developers are qualified to review the code
3. said developers have time to bother with the review
And what really is the point? What, am I supposed to figure out what arcane git command will let me merge the pull request into my local code base so I can actually review it properly, ie., test, debug single step, etc.?
Meh. The only purpose for pull requests that I've ever found is to REJECT them. Particularly useful on open source projects where you get complete idiots making code changes.
Yea I feel like it...
Particularly most code review I just glaze at them and approve... Or, in the case of this team mate I mentionned, I feel like I would like to reject them all.. but well, it ain't going nowhere...