Define a place for code review in Python workflow
http://bugs.python.org/issue9376 This issue discussed docs on the proper way to create diff on windows (as it is doesn't have the tool) for sending the patch. The current proper way is to use "svn diff" which will be replaced with "hg diff". I proposed using Rietveld like:
python -m easy_install review python -m review
But Brian said using Rietveld at all is not a good idea, and didn't want to answer what should be done for it to become good idea. Probably Brian is too busy, that's why I'd like to ask people here. What do you need or expect to happen to start using code review in Python workflow? == Intro == Small introduction for insiders not familiar with outsider's problem of maintaining patches in tracker. Please forgive the tone I write about things I dislike, but I am not devoting my life to earn a title of polite bastard (this one is obligatory disclaimer I find it still doesn't work for all people, so expect some irrelevant flame in follow-ups). Ok, for an outsider like me "svn diff" 'best practice' suxx greatly, because in non-ideal conditions (and it is about 90% of cases) patches are often needed to be edited, and reuploaded. There are usually too few insiders to review you patch, and they are usually too busy to edit a small typo, and they also deny that they need an online tool to see if patch applies clearly, so if your patch doesn't apply clearly you will be asked to check where the problem is. The "svn diff" upload story continues time after time. The problem of too few insiders is that there are too few of them, and in case your patch is somehow complicated it can enjoy some years of loneliness. During this time not only the code can change, but the codebase itself can move to some other place. Some of the few are devoting great time to review new contributions and everything could be fine, but.. there is still a big time lapse, big enough that your patches start to overlap.. Trying to get out of this maintenance nightmare you will start learning Mercurial, then Mercurial Queues, then you go some weeks practicing or.. you will just give up right away, because time is more valuable than money. If you're an insider, you can propose to review patches in ViewVC, but it is just plain wrong. Just because. Just because there are tools that do the job better. Even if they have awful usability interface. That can be improved though. By stealing templates from Gerrit. == How Rietveld helps to save time == Assuming that Python has "easy_install" packaging solution bundled by default (which it doesn't of course):
python -m easy_install review
This is run once to install the Rietveld script that is used from command like like:
python -m review
and allows you to: 1. Create issue for patch review on Rietveld site 2. Run "svn diff" 3. Upload the patch 4. Supply comment for the patch everything above in one step. To upload an updated patch, you just do:
python -m review -i XXXXX -m "log of fixes made in comparison with previous patch"
Everybody can go to Rietveld site to view either patch or the whole file code _with_ the patch. Everybody can add comments _directly_ near patch lines. Everybody receives notifications about new code review comments. As code comments are hard to keep offtopic, the signal to noise ration appears to be is quite high. The patch upload script is designed in a way that every project can tune it for their needs and place into the root of source code. "review" project at PyPI.is uploaded from source at http://bitbucket.org/techtonik/pypi-rietveld - it can be tuned to address needs specific for Python development. What do you need to start using code review for contributed Python patches? Why you wouldn't recommend this practice to outsiders? -- anatoly t.
On Mon, Jul 26, 2010 at 15:18, anatoly techtonik
http://bugs.python.org/issue9376 This issue discussed docs on the proper way to create diff on windows (as it is doesn't have the tool) for sending the patch. The current proper way is to use "svn diff" which will be replaced with "hg diff".
I proposed using Rietveld like:
python -m easy_install review python -m review
But Brian said using Rietveld at all is not a good idea, and didn't want to answer what should be done for it to become good idea. -- anatoly t.
This is not at all what I said. I do not think it is a good idea to make subtle changes to the dev FAQ to replace patch submission with Rietveld, like you had suggested. Comments on the issue tracker are not the place to debate/suggest/discuss changes in development workflow. My second sentence in that comment even says that I think Rietveld will be used in the future (hopefully sooner than later).
On Mon, 26 Jul 2010 23:18:05 +0300, anatoly techtonik
python -m review
and allows you to:
1. Create issue for patch review on Rietveld site 2. Run "svn diff" 3. Upload the patch 4. Supply comment for the patch
everything above in one step. To upload an updated patch, you just do:
python -m review -i XXXXX -m "log of fixes made in comparison with previous patch"
Everybody can go to Rietveld site to view either patch or the whole file code _with_ the patch. Everybody can add comments _directly_ near patch lines. Everybody receives notifications about new code review comments.
We do use rietveld for reviews, though (so far) not usually for small patches. That could change. So: 1) write the tool 2) upload it to pypi 3) see to what extent it gets adopted. There's nothing in your proposal that is outside of your control, as far as I can tell. (Well, except for easy_install not being in the stdlib, but that's no barrier to adoption of the proposed tool.) -- R. David Murray www.bitdance.com
Am 26.07.2010 22:18, schrieb anatoly techtonik:
Small introduction for insiders not familiar with outsider's problem of maintaining patches in tracker. Please forgive the tone I write about things I dislike, but I am not devoting my life to earn a title of polite bastard (this one is obligatory disclaimer I find it still doesn't work for all people, so expect some irrelevant flame in follow-ups).
Nice! I wish we'd all do this. We'd have so much more time to do actual work.
As code comments are hard to keep offtopic, the signal to noise ration appears to be is quite high. The patch upload script is designed in a way that every project can tune it for their needs and place into the root of source code. "review" project at PyPI.is uploaded from source at http://bitbucket.org/techtonik/pypi-rietveld - it can be tuned to address needs specific for Python development.
What do you need to start using code review for contributed Python patches? Why you wouldn't recommend this practice to outsiders?
I'd welcome any patch submitted to Rietveld for review. However, your proposed "review.py" module does not exist as far as I know, and unless someone writes it, it won't. Georg -- Thus spake the Lord: Thou shalt indent with four spaces. No more, no less. Four shall be the number of spaces thou shalt indent, and the number of thy indenting shall be four. Eight shalt thou not indent, nor either indent thou two, excepting that thou then proceed to four. Tabs are right out.
I'd welcome any patch submitted to Rietveld for review. However, your
proposed "review.py" module does not exist as far as I know, and unless
someone writes it, it won't.
Haven't personally tested that it works with Rietveld due to lack of patches sitting around, but cursory investigation suggests that reports of non-existence may have been exaggerated ;) http://pypi.python.org/pypi/review/r537 Love regards etc David Miller
Am 27.07.2010 10:54, schrieb David:
I'd welcome any patch submitted to Rietveld for review. However, your
proposed "review.py" module does not exist as far as I know, and unless someone writes it, it won't.
Haven't personally tested that it works with Rietveld due to lack of patches sitting around, but cursory investigation suggests that reports of non-existence may have been exaggerated ;)
Ah! Well, a link to this instead of tirades would have been helpful from the OP; now at least we know what he's talking about. Georg -- Thus spake the Lord: Thou shalt indent with four spaces. No more, no less. Four shall be the number of spaces thou shalt indent, and the number of thy indenting shall be four. Eight shalt thou not indent, nor either indent thou two, excepting that thou then proceed to four. Tabs are right out.
On Tue, 27 Jul 2010 13:11:48 +0200, Georg Brandl
Am 27.07.2010 10:54, schrieb David:
I'd welcome any patch submitted to Rietveld for review. However, your proposed "review.py" module does not exist as far as I know, and unless someone writes it, it won't.
Haven't personally tested that it works with Rietveld due to lack of patches sitting around, but cursory investigation suggests that reports of non-existence may have been exaggerated ;)
Ah! Well, a link to this instead of tirades would have been helpful from the OP; now at least we know what he's talking about.
Indeed. If Anatoly had said, "Hey, there's this cool tool called 'review.py' on PyPI, and I think it would improve the tracker workflow if it was used something like this....what do other people think?" *That* kind of post would have gotten a completely different, and most likely more constructive, response. Anatoly, you might want to think about the fact that at this point I suspect most people's reactions to anything you post tend to be along the lines of "oh, another Anatoly rant" and either ignore it or look immediately for what's *wrong* with what you post, instead of seeing the good suggestions that are occasionally hidden inside your rants. In other words, your negative attitude toward us results in us taking a negative attitude toward you. If you engaged positively with the community instead of negatively, you'd have a much better chance of your ideas getting a positive reception. As an author I respect a lot once said, politeness is the grease that keeps the gears of society working smoothly. Your not "wasting time" being a "polite bastard" is the equivalent of deliberately throwing sand in the gears. It is, to say the least, counter-productive to your stated goals of improving the Python workflow for yourself and others. -- R. David Murray www.bitdance.com
participants (5)
-
anatoly techtonik -
Brian Curtin -
David -
Georg Brandl -
R. David Murray