Pull Request
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
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
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
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 :thumbsup:
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
not one piece of code change should get merged to master until it has been looked at by another human being via a pull request. end of story.
-
not one piece of code change should get merged to master until it has been looked at by another human being via a pull request. end of story.
Let me differ... No one of piece of code should be merged into the master until it passed some minimal test, no human intervention is necessary at any case...
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
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
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
Let me differ... No one of piece of code should be merged into the master until it passed some minimal test, no human intervention is necessary at any case...
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
Let me tell you how we do it: 1.) developer, well, develops 2.) developer tests 3.) PR 4.) PR approved, merge to master 5.) QA tests, UAT by client if needed. 6.) Prod 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. In summary, get over yourselves.
-
Let me tell you how we do it: 1.) developer, well, develops 2.) developer tests 3.) PR 4.) PR approved, merge to master 5.) QA tests, UAT by client if needed. 6.) Prod 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. In summary, get over yourselves.
It is not me - actually do not care who sees my code... From above they decided that we have no time-to-spare to do code review...
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
-
It is not me - actually do not care who sees my code... From above they decided that we have no time-to-spare to do code review...
"The only place where Success comes before Work is in the dictionary." Vidal Sassoon, 1928 - 2012
Kornfeld Eliyahu Peter wrote:
From above they decided that we have no time-to-spare to do code review.
I have quit software shops in the past because of this mantra. If you can, I would advise the same. If you can't, then just know that this is not the shop for you. IMHO. :thumbsup:
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
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. It's just easier to say everything needs a PR.
-
I just can't get myself to agree with seemingly accepted mantra that very change / ticket implementation should go through a pull request... Please, discuss! ;P 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! :rolleyes:
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
Pull requests assume: 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.
Latest Articles:
Abusing Extension Methods, Null Continuation, and Null Coalescence Operators -
Pull requests assume: 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.
Latest Articles:
Abusing Extension Methods, Null Continuation, and Null Coalescence OperatorsYea 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...
A new .NET Serializer All in one Menu-Ribbon Bar Taking over the world since 1371!
-
Pull requests assume: 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.
Latest Articles:
Abusing Extension Methods, Null Continuation, and Null Coalescence OperatorsA pull request is a GitHub thing not git. So you would just pull the branch like normal to test.
git pull origin branch
git checkout branch
ant testThen go to GitHub, write any descriptive info you need/want to, click submit review/request changes/etc, and click merge pull request if appropriate. If you want to go full automation TravisCI has easy built-in integration with GitHub to automate builds and tests on each pull request so you don't even need to use the commands above. It'll show the results in the PR and re-run if the PR branch is updated. Of course if you're the only dev and will always be the only dev then any tool is just personal preference.
