<div dir="ltr">Hi Tom<div><br></div><div style>Here are some comments on your thoughts (again, I'm no expert or authority).</div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jun 8, 2013 at 5:07 PM, Tom Prince <span dir="ltr"><<a href="mailto:tom.prince@ualberta.net" target="_blank">tom.prince@ualberta.net</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Although being able to comment on the diff inline is very convenient, my<br>
experience is that this encourages looking at changes in a line-by-line fashion,<br>
rather than looking at the overall design.  find that having to compose the<br>
entire review at once leads to a more thoughtful review. So, while it is<br>
convenient, there is a cost to it as well.<br></blockquote><div><br></div><div style>I agree with almost all of this, but not so much with the "cost" part. I know there are many different kinds of tickets, but the majority (that I see) fall into just a few categories that I have fairly well established ways of working on (as a reviewer). 1. trivial fixes, which I tend to review entirely in github. They usually have small/simple changes to existing code and a small number of entirely new tests.  2. Addition of new functionality that's not tirivial. These tend to involve mainly new code and new tests, and again I'll usually do the whole review in github. 3. More complicated changes that have required thought about the existing code, its internal interactions, and its tests. I do superficial work on these using github (commenting on style, suggesting alternate approaches to local pieces of code, etc). Github makes this process hugely simpler and faster. But in this case I also pull down the branch and git diff it manually as well as going into files in the editor to consider changes more carefully or to search for residual methods that may no longer be used or method uses which are now no longer defined, etc. I put the result of this reviewing into the Discussion tab in github.  In all cases I make sure to pull the branch locally and run the tests (except when I'm SURE I don't need to, which is when there is ALWAYS a test failure :-)).</div>
<div style><br></div><div style>I.e., (summarizing/repeating myself) most of the time I do reviewing in github and it's a clear win (simple / in context / no need to refer to the code line or cut & paste, etc). But as you say some reviewing needs much more care and thought, done with an editor, with grep, etc. The result of that can go into the github discussion.</div>
<div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">While I understand the appeal of being able to merge with a single click (and<br>
wouldn't be opposed to a tool that does this), I'm not sure that github's<br>
implementation is desirable.  I think there is value in composing meaningful<br>
commit messages, for commits to trunk. While github supports this, it doesn't<br>
encourage it. (I've been looking through buildbot's logs recently, and most<br>
commits to master have the message "Merge branch '<branch-name>' of<br>
git://<a href="http://github.com/" target="_blank">github.com/</a><user>/buildbot"." Certainly some of this is social, I don't<br>
think github provides any tools to help enforce this.<br></blockquote><div><br></div><div style>Right, this has to be done by convention. I find it's helpful to write the issue description with an eye towards the future merging and then to cut & paste the issue description into the merge message and edit it before merging. If you're conscious of that workflow you can write an issue description whose first paragraph can easily become the merge description. We also put the names (you can use @name) of the reviewers into the merge (on a separate line, like "R: @name1 @name2") as well as the Fixes #NNNN line.  Often the merge descriptions would be just one line, as it's easy to be (or become) lazy.</div>
<div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'm not entirely sure how the hybrid workflow would make things easier. It seems<br>
like one would need to look at two places for comments rather than just one;<br>
even if all the comments on the code itself are on github, one will still need<br>
to look at the history of the ticket to see any discussion of the design or<br>
other consideration.</blockquote><div><br></div><div style>Do you mean 2 places within the one pull request?  I.e., the Files Changed and the Discussion tabs?  If so, then I understand. I got used to that pretty quickly. You can find more substantive discussion in one place (including reviewers giving +1 or saying they need more time or asking if a pull request should be withdrawn etc) and code-specific comments in the Files Changed section.   I find it very useful in the latter if the original author makes sure they follow up on each suggestion in the Files Changed section to say "Done" or "Wont do this...." etc.  This helps a returning reviewer to see that requested changes have been made without needing to look at the code again.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Potentially more, if more than one person has worked on a<br>
branch; unless everybody involved can push to the same repo, only a single<br>
person can add commits to a specific pull request.</blockquote><div><br></div><div style>But anyone can comment / review.  And if you happen to have code you want to add to the branch but you don't have the right to push into that branch (which we do only rarely), you can just make your own branch, submit a pull request into the branch you're reviewing, and mention that in the review. (Unless the Twisted process has changed, I think adding code to a branch would then rule you out as an ongoing reviewer.)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">For core developers, this<br>
could just be the main repo, but for non-core developers (or when a core<br>
developer takes over from an outside contributor), there will necessarily be<br>
multiple pull requests for a single change.<br></blockquote><div><br></div><div style>I don't think this is the right way to do things.  It makes more sense that there is one pull request. Those who can/want/dare can push into its branch. Those who cant but want to suggest other code can suggest their code is pulled into the branch in question.  Others can just put code into their reviews - I do that often, just copy a chunk of Python, wrap it in ```python  .... ``` and make a few suggested edits.</div>
<div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I wonder how much of the issue that github solves could be addressed by other<br>
means? Forcing a build is now two clicks from the ticket page, and diffs one. I<br>
just discovered <a href="https://github.com/Automattic/trac-code-comments-plugin" target="_blank">https://github.com/Automattic/trac-code-comments-plugin</a> which<br>
allows inline commenting. We could implement a tool to merge to trunk with a<br>
properly formated commit message from the web. There is, of course, the question<br>
of whether it worth the effort to implement this ourselves, when github exists.<br></blockquote><div><br></div><div style>I don't think it makes sense to try writing these things. Github is great and it moves so quickly. It's not perfect and it does leave a lot of things to coding team convention, but it's very good. In my (painful) experience, it's rarely worth writing your own stuff in a situation like this and missing out on all the other goodness that is rapidly making its way into the mainstream heavily used and very actively developed tools.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
That consideration has to be tempered with the fact that github imposes<br>
restrictions on our workflow that seem to run counter to the philosophy of<br>
twisted development.</blockquote><div><br></div><div style>I'm not sure what these are.  But, I'm barely involved in Twisted development, so it's likely just ignorance.</div><div style><br></div><div style>Thanks for the comments, mine are just my own small sample experiences and are probably sub-optimal and/or under-informed etc.</div>
<div style><br></div><div style>Terry</div><div style><br></div></div><br></div></div>