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.pri...@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
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
