Opened 6 years ago

Closed 4 years ago

#8948 closed enhancement (implemented)

Write a "code review guidelines" page

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Normal Keywords: tor-doc, 027-triaged-1-in, TorCoreTeam201509, 028-triaged
Cc: g.koppen@… Actual Points:
Parent ID: Points: small/medium
Reviewer: Sponsor: SponsorU

Description

Check out this awesome page that Twisted has:

https://twistedmatrix.com/trac/wiki/ReviewProcess

We should totally get one like it to give people a checklist of what to do when they're coding.

Child Tickets

Change History (19)

comment:1 Changed 6 years ago by gk

Cc: g.koppen@… added

comment:2 Changed 6 years ago by arma

Wonder what their (twisted's) license for the text on that page is.

comment:3 Changed 6 years ago by atagar

I'm surprised that you consider that to be a good example. If I was a contributor faced with a page saying "Both authors and reviewers should be intimately familiar with all requirements on this page." followed by that many requirements I'd be reluctant to contribute.

This isn't to say that twisted's requirements are at all bad, just that it's a lot to expect first time contributors to follow. For stem I try to make contributing damn easy. If a developer runs...

run_tests.py --all

... and it passes then they're good to send their change out for code review. That runs all the tests including style checkers and pyflakes.

In short, if tor has requirements that you want followed prior to sending code reviews then imho it should be a simple make target, not a wall of text.

comment:4 in reply to:  3 Changed 6 years ago by rransom

Replying to atagar:

In short, if tor has requirements that you want followed prior to sending code reviews then imho it should be a simple make target, not a wall of text.

Most of them are in doc/HACKING, in the ‘Patch checklist’ section. Some of the checklist items (at least “include a file in the "changes" directory as appropriate”) cannot be automated.

comment:5 Changed 6 years ago by nickm

Keywords: tor-doc added

comment:6 Changed 5 years ago by andrea

Triage for 0.2.5.x: do we have any idea which of the several things described here we want to do? The original ticket sounds like it really wants a wiki page or something, so doesn't quite seem to make sense targeting a specific release of Tor. OTOH, if we want a make target, we should probably have a clearer idea what we want it to do beyond what make check-spaces already does.

The most substantive requirement for new patches we've introduced since this is the no-new-uncovered-lines rule; it'd be nice to have an automated way to check that - diff output filtered for only chunks that add a new uncovered line, perhaps? - but we need to make the automated cov-diff tool smarter before we can do that (see #11145, #11146).

comment:7 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

IMO, I'm still fine with a wall-of-text if it's targeted at reviewers. If it's targeted at submitters, it should be shorter and simpler. I'm fine with having a list for experienced people that's more rigorous than the new-folks list.

I also like the idea of having automated checks do more, but we shouldn't IMO only put things on the review checklist that can be automated.

Also, deferring. This should happen IMO but it's indefinitely-deferrable.

comment:8 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.7.x-final

These may be worth looking at for 0.2.7.

comment:9 Changed 4 years ago by nickm

Status: newassigned

comment:10 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking some tickets as triaged-in for 0.2.7 based on early triage

comment:11 Changed 4 years ago by isabela

Keywords: SponsorU added
Points: small/medium
Priority: normalmajor
Version: Tor: 0.2.7

comment:12 Changed 4 years ago by isabela

Keywords: TorCoreTeam201507 added

comment:13 Changed 4 years ago by nickm

Owner: set to nickm

comment:14 Changed 4 years ago by nickm

Keywords: TorCoreTeam201508 added; TorCoreTeam201507 removed

comment:15 Changed 4 years ago by nickm

Keywords: TorCoreTeam201509 added; TorCoreTeam201508 removed
Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:16 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:17 Changed 4 years ago by nickm

Keywords: SponsorU removed
Sponsor: SponsorU

Bulk-replace SponsorU keyword with SponsorU field.

comment:18 Changed 4 years ago by nickm

Severity: Normal

Started doing this as doc/HACKING/HowToReview.txt

comment:19 Changed 4 years ago by nickm

Resolution: implemented
Status: assignedclosed
Note: See TracTickets for help on using tickets.