Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove requirement to find at least three problems in the code in PR #294

Closed
driver733 opened this issue Feb 21, 2018 · 13 comments
Closed

Remove requirement to find at least three problems in the code in PR #294

driver733 opened this issue Feb 21, 2018 · 13 comments

Comments

@driver733
Copy link

@driver733 driver733 commented Feb 21, 2018

From the policy.

I believe that the main goal of a code review is to assure that it does not have any problems, not that it contains problems. Otherwise, the code reviewers are enabled to come up with issues and be pedantic to an unnecessary extent, if there are, indeed, no problems in the code. I think that, instead, we should reward the code reviewer if he finds three problems or more in a PR.

@0crat
Copy link
Collaborator

@0crat 0crat commented Feb 21, 2018

@yegor256/z please, pay attention to this issue

@yegor256
Copy link
Member

@yegor256 yegor256 commented Feb 22, 2018

@driver733 any code has problems. The job of a reviewer is to find them. It's an important requirement. Without that we will easily have "code is good" reviews.

@yegor256 yegor256 closed this Feb 22, 2018
@StuporHero
Copy link

@StuporHero StuporHero commented Jun 16, 2018

@yegor256 I agree that we want to avoid "code is good" reviews, but can you clarify who the "job performer" is that is not receiving payment if the quality is bad because no problems were found? It seems to me that if you punish the DEV for that instead of the REV, the incentive is not structured properly to encourage the REV to find problems in the code.

Sorry if I should have opened a new ticket, but this ticket is linked in the policy, so I decided this is the most relevant place to bring this up.

@yegor256
Copy link
Member

@yegor256 yegor256 commented Jun 24, 2018

@StuporHero what would you propose instead? I'm not sure I got the idea..

@StuporHero
Copy link

@StuporHero StuporHero commented Jun 25, 2018

@yegor256 I’m not very clear about how it is supposed to work now, so that’s probably why I didn’t express my idea very well. I guess I need some clarification.

I’m not sure who is supposed to get points for a completed PR that passes QA. Is it the REV or the DEV? In other words, is the PR treated like a job performed by the REV that is separate from the Issue ticket assigned to the DEV when it comes to points awarded?

@yegor256
Copy link
Member

@yegor256 yegor256 commented Jun 25, 2018

@StuporHero first, we have to remember that a ticket may have many pull requests merged before it's fixed. A ticket is assigned to a DEV. The DEV is responsible to fix/close it and he/she submits pull requests in order to achieve that. How many of them will be submitted -- we don't know. The DEV will be paid once the ticket is closed.

Next, each pull request submitted has to be reviewed by a different REV. Those people must be paid for their work no matter whether pull requests are merged or rejected.

QA happens in both places. First, in the ticket. Second, in all pull requests. If the ticket doesn't pass the QA -- the DEV suffers. If a pull request doesn't pass the QA -- the REV suffers. Make sense now?

@StuporHero
Copy link

@StuporHero StuporHero commented Jun 25, 2018

@yegor256 Yes. That is how I thought it should work, but the wording of the policy was a little unclear, so I thought the DEV would suffer if a PR didn't pass QA. Thanks!

@fabriciofx
Copy link

@fabriciofx fabriciofx commented Jan 4, 2019

@yegor256 I'm sorry, but I disagree. I think perfectly possible a PR which hasn't any problem or at least one. Why exactly three? Where did you get this number?

@yegor256
Copy link
Member

@yegor256 yegor256 commented Jan 7, 2019

@fabriciofx it's just a number. We can come up with a different one. But we do need some problems to be found, right?

@fabriciofx
Copy link

@fabriciofx fabriciofx commented Jan 7, 2019

@yegor256 I'm sorry, but I disagree. Some changes (few, of course) there are nothing to comment. For example, that change in copyright year you did some time ago in Zold, it's a very simple change. What I will comment in a PR in this case?

@driver733
Copy link
Author

@driver733 driver733 commented Jan 9, 2019

@yegor256 What about changing the rule to "at least one issue/improvement must be highlighted by a reviewer"?

@fabriciofx
Copy link

@fabriciofx fabriciofx commented Jan 17, 2019

@yegor256 just to reforce my point of view: I'm doing many reviews of refactoring classes names at Cactoos. This kind of change normally has absolute nothing to comment, but I spend my time because I need check it to garantee that is ok, right? WDYT in this case the reviewer earns at least the points?

@astef
Copy link

@astef astef commented Mar 17, 2019

Natural collective reaction to this requirement would be the tendency to leave few obvious and typical defects in the code, because it is beneficial for both developer and reviewer.

Make an auction among reviewers to let them to propose their price for finding one more code enhancement in the PR. And hit the developer with penalties for each problem found. And formalize the criteria for the code problem (like asymptotic complexity of algorithm, excessive level of access modifier, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.