Page MenuHomePhabricator

Use case: Weekly reviews
Closed, DuplicatePublic

Description

Hi,

I would like to have weekly reviews for all commits in that week: Each team gets a weekly reviewer assigned.

Easiest solution for me would be an interface where revisions can be searched on date, and a review can be created for all revisions in a certain range or all revisions of a certain date range.
This could also work if it's a command-line tool, but a webinterface would be better in my opinion.

Thanks,
Michaël

Event Timeline

MichaelDeGroot assigned this task to epriestley.
MichaelDeGroot raised the priority of this task from to Needs Triage.
MichaelDeGroot updated the task description. (Show Details)
MichaelDeGroot added a project: Phabricator.
MichaelDeGroot added a subscriber: MichaelDeGroot.

By the way, an issue with your workaround on IRC is that each commit would be a separate review request so a lot of monkey click work.

The workaround:
<@epriestley> LoonaTick: a workaround would be to create a Herald rule, like:
<@epriestley> Global rule, triggers for commits, [Always], [Trigger an audit by] [username]
<@epriestley> But you'd have to remember to update it every week.
<@epriestley> It's not technically difficult to provide arc audit or similar, but there are enough missing pieces that it will probably take us a while to get there.
<LoonaTick> epriestley: Thank you very much, I think that should work. The responsibility to change this can easily be put to the developer who has to do extra audits if he forgets to change it :)

You want a single, enormous review comprising every change which has happened in an entire week?

Yes, that's correct. We're split up in several teams, +/- 10 developers per team so reviewing 1 week should cost about 2-4 hours per reviewers. This way you can plan someone to do the review instead of having a process consuming time at unknown moments.

We will probably support that eventually, but it may be a long time. Creating a single review from many commits by many authors creates other problems, as well. For example, an issue raised in review may be in code that several different commits -- with several different authors -- touched. Currently, reviews and audits have a single author, so responsibility for fixing issues is clear.

I also can't imagine reviewing 10 developer-weeks of work in only 4 hours -- that's a ratio of 100:1! -- or 10 developer-weeks of changes in a single huge commit -- it would be impossible for me to understand. Is there something unusual about the work you do which allows review to be conducted so quickly? Is the weekly review looking only for specific issues (e.g., unsafe patterns) rather than looking at changes as a whole?

How do you currently raise issues from weekly review and see that they're fixed by the appropriate parties?

You could manually create such a review by running arc diff <commit> --create, where <commit> is the hash of the last commit you reviewed a week ago, but there will probably be a lot of rough edges where support is missing for huge aggregates by multiple authors.

The plan is to setup 2 different types of reviews. The weekly review is for any commit into the trunk. Bigger features get developed in branches, when a developer is ready to commit this branch into the trunk a pre-commit review is being done (or possibly even during developing on this branch) by a senior developer.
At another company I've seen this working 6 developers in 1-3 hours where we didn't work with branches.The issue there was that some junior developers were only able to find basic mistakes (coding standards, typo, etc...) so we decided to team the junior with a senior to get the most out of it.
Here the idea is that a senior reviews the branch, so big architectural changes are always reviewed by a senior.

The fix to the target to raise the issues to is simple: Raise the issue to the one who last committed the line. This will also save review, issue creation, etc etc etc time if a developer notices he created a bug or notices he made a typo in a config file name.

Would it be possible that you create a review for me with that command for +/- 100 commits to Phabricator? We didn't set it up yet, I hope we will be able to decide with just demo versions.

Oh by the way, the 'normal' commits in the trunk are mainly bugfixes and small features, so therefor I think it can be done in so little time.

D7339 has the last 100 commits to Phabricator.

Thanks for adding the review, I think this would work perfectly for us, assuming it's fairly easy to add some javascript that asynchronously opens all the commits.

There should be a link on the page to open the diffs already, it might just be hard to spot -- "Show All Files Inline" here:

Screen_Shot_2013-10-11_at_11.22.35_AM.png (225×975 px, 22 KB)

Did you want one to click all the "(More)" links on the commit messages? Or was the diffs what you meant?

The show all files inline is indeed what I was looking for! :)

Cool. I'm going to merge this into T2487, which covers the eventual web UI version.