search

clearpool-finance / developers

Welcome to Clearpool Developers space.
0 stars 0 forks source link

Problem: the difference between "reject" and "comment" is not clear #3

Open zolotokrylin opened 4 months ago

zolotokrylin commented 4 months ago

I'm trying to follow the rules and avoid where is necessary the reject state

you don't need to avoid things. It is binary:

  • reject: if author have done a mistake
  • comment: when you think something might be wrong, but you are not 100% sure
  • approve: when you agree with everything

Originally posted by @zolotokrylin in https://github.com/clearpool-finance/clearpool.finance/issues/2622#issuecomment-2117298958

georgeciubotaru commented 4 months ago

@zolotokrylin I don't fully agree with you on that.

  1. What we are trying to accomplish by tracking reject and comment?
  2. Do we only analyze the PR owners with rejects, or we should analyze and reviewers that gives too many rejects?

For me we should analyze both cases as we want a developer to write quality code and perform accurate review.

So:

reject - for mistakes, no met requirement, not met code rules comment - for general comments, questions, or props(enhancements not related to code practices) approve - when you agree with everything

zolotokrylin commented 4 months ago

@georgeciubotaru

What we are trying to accomplish by tracking reject and comment?

We are tracking if contributors do review the code: "reject", "comment" and "approve" are contributing into "review given" counter.

Do we only analyze the PR owners with rejects, or we should analyze and reviewers that gives too many rejects?

We analyze PR owners with rejects. If the owner doesn't agree with the rejection, he/she must dispute the rejection.

comment - for general comments, questions, or props(enhancements not related to code practices)

Code review is not for discussions or unrelated conversations, and not for sidetracking or scope-increasing new propositions. Code review is about ensuring that introduced changes meet the definition of DONE.

georgeciubotaru commented 4 months ago

@zolotokrylin

We analyze PR owners with rejects. If the owner doesn't agree with the rejection, he/she must dispute the rejection.

A disputed rejection counter doesn't get canceled after that.

Code review is not for discussions or unrelated conversations, and not for sidetracking or scope-increasing new propositions. Code review is about ensuring that introduced changes meet the definition of DONE.

For sure we must not question unrelated topics, but as you mentioned, you cannot ensure that all changes meet the definition of DONE without asking additional questions to help you achieve 100% understanding.

I have a clear understanding of when and which the review state should be provided, I'm open to discussing it in a retrospective call if needed.

zolotokrylin commented 4 months ago

A disputed rejection counter doesn't get canceled after that.

The point here if the developer (anyone) rejects PRs which are not supposed to rejected, then this developer is a bad contributor. Each team member supposed to bring value, not the opposite. So, if someone makes a comment, which is not related to rejection, the PR must not be rejected.

I don't understand where the confusion comes from. Can you clearly explain, please.