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

Add info about Pre-merge Code Reviews in the "contributing to Piwik" guide #7210

Closed
mattab opened this issue Feb 15, 2015 · 2 comments
Closed
Labels
c: Website matomo.org For issues related to our matomo.org website. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.

Comments

@mattab
Copy link
Member

mattab commented Feb 15, 2015

The goal of this issue is to note in the guide "Contributing to piwik core" at http://developer.piwik.org/guides/contributing-to-piwik-core that we use Pre-merge code reviews, that issues can be tagged with Needs review tag. Maybe we could note in this guide the Rules / Benefits / Pitfalls (see #6916).

@mattab mattab added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Feb 15, 2015
@mattab mattab added this to the Short term milestone Feb 15, 2015
@mattab
Copy link
Member Author

mattab commented Feb 20, 2015

Interesting article How to write the perfect pull request? that we could link to, or reuse content from, in our contributing guide

also:

  • Take enough time for a proper, slow review, 30-60 min.
  • Authors should annotate source code before the review begins.
  • Establish quantifiable goals for code review and capture metrics so you can improve your processes.
  • Checklists substantially improve results for both authors and reviewers.
  • Verify that defects are actually fixed!
  • Authors must foster a good code review culture in which finding defects is viewed positively.
  • Beware the “Big Brother” effect.
  • The Ego Effect: Do at least some code review, even if you don’t have time to review it all
  • Lightweight-style code reviews are efficient, practical, and effective at finding bugs.

@mattab
Copy link
Member Author

mattab commented Mar 13, 2015

Very interesting: The Pitfalls of Code Review (And How To Fix Them)

In particular this:

I use a three-tier system for code reviews. Comments that begin with “You might…” indicate that I am making a suggestion, based on my experience. The developer can choose to ignore my suggestion or not, based on their experience. “You should…” comments indicate something I feel strongly about. The developer is strongly encouraged to consider the suggestion, but again, I leave this to their discretion.

Comments beginning with “You must…” are not optional. But these kinds of comments are reserved for security vulnerabilities, coding standard violations, obvious bugs, or other serious code problems. These are the only blocking code review comments, meaning that all other code review comments can be ignored, but these must be dealt with to obtain my approval for the patch. Everything else gets a thumbs-up.

--> What do you think of adopting this system for our pre-merge code reviews?

@mattab mattab modified the milestones: 3.0.0, Short term Aug 16, 2015
@mattab mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Oct 22, 2015
@mattab mattab modified the milestones: 2.16.x, 3.0.0 Feb 8, 2016
@mattab mattab added the c: Website matomo.org For issues related to our matomo.org website. label Mar 14, 2016
@mattab mattab modified the milestones: 2.16.x (LTS), 3.0.0 Apr 1, 2016
@mattab mattab removed the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Oct 21, 2019
@mattab mattab closed this as not planned Won't fix, can't repro, duplicate, stale Dec 10, 2023
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Website matomo.org For issues related to our matomo.org website. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

2 participants