[Twisted-Python] github, again

[Terry Jones]

Hi Tom

Here are some comments on your thoughts (again, I'm no expert or authority).

On Sat, Jun 8, 2013 at 5:07 PM, Tom Prince <tom.prince at ualberta.net> wrote:

> Although being able to comment on the diff inline is very convenient, my
> experience is that this encourages looking at changes in a line-by-line
> fashion,
> rather than looking at the overall design.  find that having to compose the
> entire review at once leads to a more thoughtful review. So, while it is
> convenient, there is a cost to it as well.
>

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 :-)).

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.


> While I understand the appeal of being able to merge with a single click
> (and
> wouldn't be opposed to a tool that does this), I'm not sure that github's
> implementation is desirable.  I think there is value in composing
> meaningful
> commit messages, for commits to trunk. While github supports this, it
> doesn't
> encourage it. (I've been looking through buildbot's logs recently, and most
> commits to master have the message "Merge branch '<branch-name>' of
> git://github.com/<user>/buildbot"." Certainly some of this is social, I
> don't
> think github provides any tools to help enforce this.
>

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.


> I'm not entirely sure how the hybrid workflow would make things easier. It
> seems
> like one would need to look at two places for comments rather than just
> one;
> even if all the comments on the code itself are on github, one will still
> need
> to look at the history of the ticket to see any discussion of the design or
> other consideration.


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.


> Potentially more, if more than one person has worked on a
> branch; unless everybody involved can push to the same repo, only a single
> person can add commits to a specific pull request.


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.)


> For core developers, this
> could just be the main repo, but for non-core developers (or when a core
> developer takes over from an outside contributor), there will necessarily
> be
> multiple pull requests for a single change.
>

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.


> I wonder how much of the issue that github solves could be addressed by
> other
> means? Forcing a build is now two clicks from the ticket page, and diffs
> one. I
> just discovered https://github.com/Automattic/trac-code-comments-pluginwhich
> allows inline commenting. We could implement a tool to merge to trunk with
> a
> properly formated commit message from the web. There is, of course, the
> question
> of whether it worth the effort to implement this ourselves, when github
> exists.
>

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.


> That consideration has to be tempered with the fact that github imposes
> restrictions on our workflow that seem to run counter to the philosophy of
> twisted development.


I'm not sure what these are.  But, I'm barely involved in Twisted
development, so it's likely just ignorance.

Thanks for the comments, mine are just my own small sample experiences and
are probably sub-optimal and/or under-informed etc.

Terry
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://twistedmatrix.com/pipermail/twisted-python/attachments/20130609/1990005d/attachment.html>