Re: [python-committers] [Core-mentorship] Regarding reviewing test cases written for tabnanny module
Thanks for the clarification. We should probably move this discussion to the python-committers list rather than core-mentorship.
On Mon, Apr 10, 2017 at 12:12 PM, Terry Reedy <tjreedy@udel.edu> wrote:
On 4/10/2017 12:54 PM, Guido van Rossum wrote:
So the response from Martin Panter (https://github.com/python/cpython/pull/851#issuecomment-292755992) sounds like he's not set up for the new GitHub workflow. I'm CC'ing Martin here.
The specific issue Martin raised is "Sorry but I don’t have an easy way to see your fixes relative to the old version I reviewed." If the original and modified patches were posted in proper format to b.p.o., then one could hit [review] to start Rietveld and request a side-by-side diff of the two versions. This is perfect for reviewing responses to comments, especially those made in-line. For this issue, Martin made about 20 inline comments.
I don't see any way to get the equivalent on a github PR. It appears that the original patch is replaced by the revised patch. To me, Rietveld was a great review tool and its loss a regression in the work process. I hope that this can be fixed somehow.
tjr
Core-mentorship mailing list Core-mentorship@python.org https://mail.python.org/mailman/listinfo/core-mentorship
-- --Guido van Rossum (python.org/~guido)
If someone makes a review on github (as opposed to a simple comment) I believe the state of the code as it was when that review as made can be viewed by hitting the “View Changes” button next to that review in the timeline.
On Apr 10, 2017, at 3:18 PM, Guido van Rossum <guido@python.org> wrote:
Thanks for the clarification. We should probably move this discussion to the python-committers list rather than core-mentorship.
On Mon, Apr 10, 2017 at 12:12 PM, Terry Reedy <tjreedy@udel.edu <mailto:tjreedy@udel.edu>> wrote: On 4/10/2017 12:54 PM, Guido van Rossum wrote: So the response from Martin Panter (https://github.com/python/cpython/pull/851#issuecomment-292755992 <https://github.com/python/cpython/pull/851#issuecomment-292755992>) sounds like he's not set up for the new GitHub workflow. I'm CC'ing Martin here.
The specific issue Martin raised is "Sorry but I don’t have an easy way to see your fixes relative to the old version I reviewed." If the original and modified patches were posted in proper format to b.p.o., then one could hit [review] to start Rietveld and request a side-by-side diff of the two versions. This is perfect for reviewing responses to comments, especially those made in-line. For this issue, Martin made about 20 inline comments.
I don't see any way to get the equivalent on a github PR. It appears that the original patch is replaced by the revised patch. To me, Rietveld was a great review tool and its loss a regression in the work process. I hope that this can be fixed somehow.
tjr
Core-mentorship mailing list Core-mentorship@python.org <mailto:Core-mentorship@python.org> https://mail.python.org/mailman/listinfo/core-mentorship <https://mail.python.org/mailman/listinfo/core-mentorship>
-- --Guido van Rossum (python.org/~guido <http://python.org/~guido>)
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
— Donald Stufft
"View Changes" doesn't work when commits in PR were squashed, which seems to be the case in https://github.com/python/cpython/pull/851
I wonder if there is a way to unsquash the commits? Will it help with reviewing this PR?
Mariatta Wijaya
On Tue, Apr 11, 2017 at 2:55 AM, Donald Stufft <donald@stufft.io> wrote:
If someone makes a review on github (as opposed to a simple comment) I believe the state of the code as it was when that review as made can be viewed by hitting the “View Changes” button next to that review in the timeline.
On Apr 10, 2017, at 3:18 PM, Guido van Rossum <guido@python.org> wrote:
Thanks for the clarification. We should probably move this discussion to the python-committers list rather than core-mentorship.
On Mon, Apr 10, 2017 at 12:12 PM, Terry Reedy <tjreedy@udel.edu> wrote:
On 4/10/2017 12:54 PM, Guido van Rossum wrote:
So the response from Martin Panter (https://github.com/python/cpython/pull/851#issuecomment-292755992) sounds like he's not set up for the new GitHub workflow. I'm CC'ing Martin here.
The specific issue Martin raised is "Sorry but I don’t have an easy way to see your fixes relative to the old version I reviewed." If the original and modified patches were posted in proper format to b.p.o., then one could hit [review] to start Rietveld and request a side-by-side diff of the two versions. This is perfect for reviewing responses to comments, especially those made in-line. For this issue, Martin made about 20 inline comments.
I don't see any way to get the equivalent on a github PR. It appears that the original patch is replaced by the revised patch. To me, Rietveld was a great review tool and its loss a regression in the work process. I hope that this can be fixed somehow.
tjr
Core-mentorship mailing list Core-mentorship@python.org https://mail.python.org/mailman/listinfo/core-mentorship
-- --Guido van Rossum (python.org/~guido)
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
— Donald Stufft
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On 11 April 2017 at 13:13, Mariatta Wijaya <mariatta.wijaya@gmail.com> wrote:
"View Changes" doesn't work when commits in PR were squashed, which seems to be the case in https://github.com/python/cpython/pull/851
I wonder if there is a way to unsquash the commits? Will it help with reviewing this PR?
Mariatta Wijaya
On Tue, Apr 11, 2017 at 2:55 AM, Donald Stufft <donald@stufft.io> wrote:
If someone makes a review on github (as opposed to a simple comment) I believe the state of the code as it was when that review as made can be viewed by hitting the “View Changes” button next to that review in the timeline.
On Apr 10, 2017, at 3:18 PM, Guido van Rossum <guido@python.org> wrote:
Thanks for the clarification. We should probably move this discussion to the python-committers list rather than core-mentorship.
On Mon, Apr 10, 2017 at 12:12 PM, Terry Reedy <tjreedy@udel.edu> wrote:
On 4/10/2017 12:54 PM, Guido van Rossum wrote:
So the response from Martin Panter (https://github.com/python/cpython/pull/851#issuecomment-292755992) sounds like he's not set up for the new GitHub workflow. I'm CC'ing Martin here.
The specific issue Martin raised is "Sorry but I don’t have an easy way to see your fixes relative to the old version I reviewed." If the original and modified patches were posted in proper format to b.p.o., then one could hit [review] to start Rietveld and request a side-by-side diff of the two versions. This is perfect for reviewing responses to comments, especially those made in-line. For this issue, Martin made about 20 inline comments.
I don't see any way to get the equivalent on a github PR. It appears that the original patch is replaced by the revised patch. To me, Rietveld was a great review tool and its loss a regression in the work process. I hope that this can be fixed somehow.
In this particular pull request, I think the submitter has rebased their commit, and force-pushed it. These days, I notice Git Hub seems to forget old commits pretty soon after you force-push the branch they are on. I don't think you can "unsquash" them retrospectively; you would need a copy of the old commits saved somewhere.
Other times people add revised commits on top of their old commits, which would have been easier for me in this situation, but I suspect that makes it harder for the person pushing the final change if they have to squash it into a single commit. (I noticed the eventual commit message is often messy, redundant, automatically generated, etc.)
On Mon, Apr 10, 2017 at 8:55 PM, Martin Panter <vadmium+py@gmail.com> wrote:
In this particular pull request, I think the submitter has rebased their commit, and force-pushed it. These days, I notice Git Hub seems to forget old commits pretty soon after you force-push the branch they are on. I don't think you can "unsquash" them retrospectively; you would need a copy of the old commits saved somewhere.
Other times people add revised commits on top of their old commits, which would have been easier for me in this situation, but I suspect that makes it harder for the person pushing the final change if they have to squash it into a single commit. (I noticed the eventual commit message is often messy, redundant, automatically generated, etc.)
We have some of this in the mypy project (though it's a much smaller project). The workflow that I like best (not all contributors use it) is
When the contributor makes multiple local commits without pushing to the PR, I recommend using --amend unless they have several commits that actually are logically distinct and relevant to the reviewer. (--amend is especially important when fixing lint bugs or typos).
Please don't use --amend across pushes to the PR
It's OK to just pull+rebase and then use git push -f, but honestly pull+merge is fine too
When responding to a review please NEVER use commit --amend, that's where reviewing becomes painful
It's up to the reviewer who merges to rewrite the commit message: the reviewer usually has a much better sense for what info in the commit message will still be interesting a month or a year from now than the contributor. (Often just copying the original comment from the top of the PR is adequate.)
In mypy I recently disabled merge commits in mypy because some other committers sometimes accidentally used it, and the history becomes really ugly. (Maybe Brett already did this for cpython.)
I do agree that Rietveld's workflow is usually much more convenient, with the exception of what it does around merging or rebasing other diffs.
-- --Guido van Rossum (python.org/~guido)
On Mon, 10 Apr 2017 21:53:44 -0700, Guido van Rossum <guido@python.org> wrote:
When the contributor makes multiple local commits without pushing to the PR, I recommend using --amend unless they have several commits that actually are logically distinct and relevant to the reviewer. (--amend is especially important when fixing lint bugs or typos).
Please don't use --amend across pushes to the PR
It's OK to just pull+rebase and then use git push -f, but honestly pull+merge is fine too
When responding to a review please NEVER use commit --amend, that's where reviewing becomes painful
In the devguide PR addressing this issue, I suggested we just say that one should never force push to the PR. I can see that a force push after a rebase *before* addressing review comments would be fine, but maybe it would be better to prefer merge since we're going to squash at the end anyway.
- It's up to the reviewer who merges to rewrite the commit message: the reviewer usually has a much better sense for what info in the commit message will still be interesting a month or a year from now than the contributor. (Often just copying the original comment from the top of the PR is adequate.)
Some of our core committers need to learn to do this :)
--David
On 12 April 2017 at 00:21, R. David Murray <rdmurray@bitdance.com> wrote:
On Mon, 10 Apr 2017 21:53:44 -0700, Guido van Rossum <guido@python.org> wrote:
When the contributor makes multiple local commits without pushing to the PR, I recommend using --amend unless they have several commits that actually are logically distinct and relevant to the reviewer. (--amend is especially important when fixing lint bugs or typos).
Please don't use --amend across pushes to the PR
It's OK to just pull+rebase and then use git push -f, but honestly pull+merge is fine too
When responding to a review please NEVER use commit --amend, that's where reviewing becomes painful
In the devguide PR addressing this issue, I suggested we just say that one should never force push to the PR. I can see that a force push after a rebase *before* addressing review comments would be fine, but maybe it would be better to prefer merge since we're going to squash at the end anyway.
Yeah, Serhiy made this point when we were working on the instructions for committers editing a contributor's PR branch: https://docs.python.org/devguide/gitbootcamp.html#editing-a-pull-request-pri...
Since we do the squash & merge to get an atomic commit at the end, it doesn't make sense to do any force pushes along the way.
- It's up to the reviewer who merges to rewrite the commit message: the reviewer usually has a much better sense for what info in the commit message will still be interesting a month or a year from now than the contributor. (Often just copying the original comment from the top of the PR is adequate.)
Some of our core committers need to learn to do this :)
Technically coming up with a meaningful message when committing patches isn't a new requirement, but I admit it's a lot easier to forget to clean things up when there's a big green button right in front of you waiting to be pushed :)
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Tue, Apr 11, 2017 at 7:42 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
Since we do the squash & merge to get an atomic commit at the end, it doesn't make sense to do any force pushes along the way.
I was going to argue with this, but then I realized you're right. We shouldn't need rebase any more, merge should be fine for all situations. (I can think of exceptions but they aren't worth mentioning.)
-- --Guido van Rossum (python.org/~guido)
On 12 April 2017 at 01:57, Guido van Rossum <guido@python.org> wrote:
On Tue, Apr 11, 2017 at 7:42 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
Since we do the squash & merge to get an atomic commit at the end, it doesn't make sense to do any force pushes along the way.
I was going to argue with this, but then I realized you're right. We shouldn't need rebase any more, merge should be fine for all situations. (I can think of exceptions but they aren't worth mentioning.)
That was exactly my reaction when Serhiy pointed it out - I started to argue the point, but then invalidated all my own arguments before actually posting anything :)
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 14.04.17 17:02, Nick Coghlan wrote:
On 12 April 2017 at 01:57, Guido van Rossum <guido@python.org> wrote:
On Tue, Apr 11, 2017 at 7:42 AM, Nick Coghlan <ncoghlan@gmail.com> wrote:
Since we do the squash & merge to get an atomic commit at the end, it doesn't make sense to do any force pushes along the way.
I was going to argue with this, but then I realized you're right. We shouldn't need rebase any more, merge should be fine for all situations. (I can think of exceptions but they aren't worth mentioning.)
That was exactly my reaction when Serhiy pointed it out - I started to argue the point, but then invalidated all my own arguments before actually posting anything :)
I don't remember I said anything about this.
On 15 April 2017 at 03:15, Serhiy Storchaka <storchaka@gmail.com> wrote:
On 14.04.17 17:02, Nick Coghlan wrote:
That was exactly my reaction when Serhiy pointed it out - I started to argue the point, but then invalidated all my own arguments before actually posting anything :)
I don't remember I said anything about this.
My apologies, I misremembered who pointed that out to me. It was actually Inada-san, here: https://github.com/python/devguide/issues/129#issuecomment-281910027
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Mon, Apr 10, 2017 at 8:55 PM, Martin Panter <vadmium+py@gmail.com> wrote:
In this particular pull request, I think the submitter has rebased their commit, and force-pushed it. These days, I notice Git Hub seems to forget old commits pretty soon after you force-push the branch they are on. I don't think you can "unsquash" them retrospectively; you would need a copy of the old commits saved somewhere.
In the past, after force-pushing on GitHub, I've noticed that "orphaned" commits can still be accessed as long as you remember the previous URL / SHA. For example, in this case, clicking "View Changes" goes to an URL that doesn't show anything:
https://github.com/python/cpython/pull/851/files/d66ae892c51ab84eac71a3f1b55...
While the commit isn't visible under the pull-request URL, it can still be accessed through the following two URLs (in the proposer's repo and in the target repo):
https://github.com/ultimatecoder/cpython/commit/d66ae892c51ab84eac71a3f1b558... https://github.com/python/cpython/commit/d66ae892c51ab84eac71a3f1b558a021a9c...
I'm not sure if this helps though because the inline comments don't seem to be present in this URL.
--Chris
Other times people add revised commits on top of their old commits, which would have been easier for me in this situation, but I suspect that makes it harder for the person pushing the final change if they have to squash it into a single commit. (I noticed the eventual commit message is often messy, redundant, automatically generated, etc.)
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On 4/10/2017 11:55 PM, Martin Panter wrote:
On 11 April 2017 at 13:13, Mariatta Wijaya <mariatta.wijaya@gmail.com> wrote:
"View Changes" doesn't work when commits in PR were squashed, which seems to be the case in https://github.com/python/cpython/pull/851
I wonder if there is a way to unsquash the commits? Will it help with reviewing this PR?
In this particular pull request, I think the submitter has rebased their commit, and force-pushed it. These days, I notice Git Hub seems to forget old commits pretty soon after you force-push the branch they are on. I don't think you can "unsquash" them retrospectively; you would need a copy of the old commits saved somewhere.
Other times people add revised commits on top of their old commits, which would have been easier for me in this situation, but I suspect that makes it harder for the person pushing the final change if they have to squash it into a single commit. (I noticed the eventual commit message is often messy, redundant, automatically generated, etc.)
I was under the impression that the green [commit] button would do the squashing. Or at least that it could.
tjr
On Apr 11, 2017, at 12:25 PM, Terry Reedy <tjreedy@udel.edu> wrote:
On 4/10/2017 11:55 PM, Martin Panter wrote:
On 11 April 2017 at 13:13, Mariatta Wijaya <mariatta.wijaya@gmail.com> wrote:
"View Changes" doesn't work when commits in PR were squashed, which seems to be the case in https://github.com/python/cpython/pull/851
I wonder if there is a way to unsquash the commits? Will it help with reviewing this PR?
In this particular pull request, I think the submitter has rebased their commit, and force-pushed it. These days, I notice Git Hub seems to forget old commits pretty soon after you force-push the branch they are on. I don't think you can "unsquash" them retrospectively; you would need a copy of the old commits saved somewhere.
Other times people add revised commits on top of their old commits, which would have been easier for me in this situation, but I suspect that makes it harder for the person pushing the final change if they have to squash it into a single commit. (I noticed the eventual commit message is often messy, redundant, automatically generated, etc.)
I was under the impression that the green [commit] button would do the squashing. Or at least that it could.
Yes it can, and IIRC for CPython we have it set so it _only_ does that. Although the commit message may be ugly if you don’t adjust it in the text editor that pops up when GitHub asks you to confirm the merge since it by default just concats all of the commit messages into a list so you might get a commit message like:
implement feature
fix thing
ugh
address review
Instead of a nice clean one. That’s going to be up to the person hitting the merge button to edit the commit message to be clean though.
— Donald Stufft
On 4/11/2017 1:21 PM, Donald Stufft wrote:
On Apr 11, 2017, at 12:25 PM, Terry Reedy <tjreedy@udel.edu <mailto:tjreedy@udel.edu>> wrote:
I was under the impression that the green [commit] button would do the squashing. Or at least that it could.
Yes it can, and IIRC for CPython we have it set so it _only_ does that. Although the commit message may be ugly if you don’t adjust it in the text editor that pops up when GitHub asks you to confirm the merge since it by default just concats all of the commit messages into a list so you might get a commit message like:
implement feature
fix thing
ugh
address review
Instead of a nice clean one. That’s going to be up to the person hitting the merge button to edit the commit message to be clean though.
I think committers should always be responsible for the commit message. This will usually mean editing submissions from non-committers. Since message is truncated to first line in some displays, the latter should summarize main point of commit.
So there is already a PR in the devguide advising contributors to keep their commit history intact, see https://github.com/python/devguide/pull/162 Assuming people do read the devguide, then let's hope that we won't lose the commit history going forward.
There is also already a section in the devguide instructing core devs to clean up commit message, see http://cpython-devguide.re adthedocs.io/gitbootcamp.html#accepting-and-merging-a-pull-request If this is not clear enough, please propose a PR or file an issue.
Back to the original issue with reviewing the PR https://github.com/python/ cpython/pull/851
Other than not being able to review the diff, is there any other problem? Can the PR be reviewed as is?
Martin, you said: "I’m not really set up to properly review and push stuff with the new Git Hub setup," Are you wanting to make edits to the PR yourself? If so, maybe you can try these instructions: http://cpython-devguide. readthedocs.io/gitbootcamp.html#editing-a-pull-request-prior-to-merging
Mariatta Wijaya
On Tue, Apr 11, 2017 at 11:30 AM, Terry Reedy <tjreedy@udel.edu> wrote:
On 4/11/2017 1:21 PM, Donald Stufft wrote:
On Apr 11, 2017, at 12:25 PM, Terry Reedy <tjreedy@udel.edu
<mailto:tjreedy@udel.edu>> wrote:
I was under the impression that the green [commit] button would do the
squashing. Or at least that it could.
Yes it can, and IIRC for CPython we have it set so it _only_ does that. Although the commit message may be ugly if you don’t adjust it in the text editor that pops up when GitHub asks you to confirm the merge since it by default just concats all of the commit messages into a list so you might get a commit message like:
implement feature
fix thing
ugh
address review
Instead of a nice clean one. That’s going to be up to the person hitting the merge button to edit the commit message to be clean though.
I think committers should always be responsible for the commit message. This will usually mean editing submissions from non-committers. Since message is truncated to first line in some displays, the latter should summarize main point of commit.
python-committers mailing list python-committers@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On 12 April 2017 at 03:10, Mariatta Wijaya <mariatta.wijaya@gmail.com> wrote:
Back to the original issue with reviewing the PR https://github.com/python/cpython/pull/851
Other than not being able to review the diff, is there any other problem? Can the PR be reviewed as is?
Martin, you said: "I’m not really set up to properly review and push stuff with the new Git Hub setup," Are you wanting to make edits to the PR yourself? If so, maybe you can try these instructions: http://cpython-devguide.readthedocs.io/gitbootcamp.html#editing-a-pull-reque...
At the moment I don’t have the setup nor time to spend on this, sorry. If I were in a position to download the pull request and properly review and test it out, I may want to negotiate changes with the original contributor, or failing that, make edits myself, but i’m not intending to do this right now.
On Tue, Apr 11, 2017 at 11:30 AM, Terry Reedy <tjreedy@udel.edu> wrote:
I think committers should always be responsible for the commit message. This will usually mean editing submissions from non-committers. Since message is truncated to first line in some displays, the latter should summarize main point of commit.
participants (9)
-
Chris Jerdonek -
Donald Stufft -
Guido van Rossum -
Mariatta Wijaya -
Martin Panter -
Nick Coghlan -
R. David Murray -
Serhiy Storchaka -
Terry Reedy