Re: [CONF] DAFFODIL > Code Contributor Workflow

[Steve Lawrence <sl...@apache.org>]

On 11/17/2017 08:45 AM, Michael Beckerle (Confluence) wrote:
> <https://cwiki.apache.org/confluence/display/~mbeckerle?src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147> 
> 	Michael Beckerle *commented* on a page
> 
> comment icon 
> <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?focusedCommentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=view#comment-74687418> 
> 
> 
> 	Re: Code Contributor Workflow 
> <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?focusedCommentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=view#comment-74687418> 
> 
> 
> I want to fold these learnings into the flow above.
> 
> Many of our patch sets are large. We're still refactoring the internals of 
> Daffodil to improve maintainability and performance. So a patch set might modify 
> 80 files. Generally review for large sets of changes like this requires several 
> iterations of a review-fix cycle.
> 
> So what we learned is that each time you want code reviewed, you want to push 
> (without --force) a single separate new commit to your branch. This commit 
> should not be squashed together with any commit from a prior review, but should 
> squash together all commits for changes since the prior review. Your work may go 
> through several cycles of commit, push, get review comments, make changes to 
> respond to them (doing local commits as often as you want), squash local 
> commmits into a "next review commit", push, repeat.
> 
> Let's say your review-fix cycle takes 3 iterations. Then at the end there should 
> be 3 commits on that branch that become part of the pull-request for review. 
> Each commit gathers comments as part of reviewing, and the response to those 
> comments is a new separate commit on the branch.
> 
> Many developers use a "commit often" discipline. Those commits are to your local 
> clone of your fork repository. Commit as often as you want there. When it is 
> time to code review, squash all the commits together into a single commit of 
> changes *since the prior review*.
> 
>   * It's very important that each time you add a new commit for review, that it
>     be separate, not squashed into anything already reviewed before it. This
>     preserves the commentary on prior commits for your pull request. It allows
>     reviewers to see how your new changes addressed the prior comments.
>   * It's very important that you will never need to "git push --force ...."
>     anything during the review-fix cycle. If you do, you have done something
>     wrong - like squashed reviewed-commits together with post-review ones.
> 
> When review comments from two reviewers come back +1, then it is time to 
> incorporate the change into the master branch.
> 
> At this point, squash together all the review commits so you have one commit.
> 
> This one you must 'git push --force' to your branch on origin (aka your fork repo).
> 
>   * Note: doing this will make it impossible to revisit review comments from the
>     commits that have been squashed together - basically the commentary is lost
>     once the commit is squashed together.
> 
> That code-review UI where you can see changes and comments interleaved into the 
> code,... that UI no longer can retrieve those comments once the commits they are 
> on have been squashed together.
> 
> That means review comments are *not a matter of permanent record* - though every 
> one is sent to the dev mailing list, so they're recorded in that way, but you 
> won't be able to open the pull request and revisit comments on the individual 
> review commits any longer.
> 
> This means we need to follow this policy:
> 
>   * If a comment contains any description/discussion that wants to be
>     maintained/remembered, it should be edited into a comment in the code
>     (perhaps with a TODO or FIXME tag so it's easy to find.)
> 
> That means when doing code review - it's useful to remind contributors to put 
> things into code comments.
> 
> Reply Icon 
> <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?replyToComment=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=reply#comment-74687418> 
> 	Reply 
> <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?replyToComment=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=reply#comment-74687418> 
> 	•
> 
> Like Icon 
> <https://cwiki.apache.org/confluence/plugins/likes/like.action?contentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=like> 
> 	Like 
> <https://cwiki.apache.org/confluence/plugins/likes/like.action?contentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=like> 
> 
> 
> Stop watching page 
> <https://cwiki.apache.org/confluence/users/removepagenotification.action?pageId=74683912&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=stop-watching> 
> 	•
> 
> Manage notifications 
> <https://cwiki.apache.org/confluence/users/editmyemailsettings.action?src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=manage> 
> 
> 
> 	
> Confluence logo big
> 
> This message was sent by Atlassian Confluence 5.8.17
> 

I think this is a really important note about how to do code reviews,
probably belongs either in the Code Contributor Workflow (not just a
comment, people may not see it) or a new page about how to do reviews. I
think this can all be summarized into a few bullet points about code
reviews and fit in the workflow page somewhere.