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!
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.
