| Submitter | via Mercurial-devel |
|---|---|
| Date | June 20, 2017, 9:45 p.m. |
| Message ID | <f86d21c457209f5f5a13.1497995157@martinvonz.svl.corp.google.com> |
| Download | mbox | patch |
| Permalink | /patch/21545/ |
| State | Accepted |
| Headers | show |
Comments
Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> writes: > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com> > # Date 1497992441 25200 > # Tue Jun 20 14:00:41 2017 -0700 > # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 > # Parent 0906385672d754f13a512fe70759f0463a069f1e > py3: catch StopIteration from next() in generatorset > > IIUC, letting the StopIteration through would not cause any bugs, but > not doing it makes the test-py3-commands.t pass. > > I have also diligently gone through all uses of next() in our code > base. They either: > > * are not called from a generator > * pass a default value to next() > * catch StopException > * work on infinite iterators > * request a fixed number of items that matches the generated number > * are about batching in wireproto which I didn't quite follow > > I'd appreciate if Augie or someone else could take a look at the > wireproto batching and convince themselves that the next(batchable) > calls there will not raise a StopIteration. I was just thinking of doing something like this. Shouldn't we just 'return None' in Mercurial instead of 'raise StopIteration'?
On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: > Martin von Zweigbergk via Mercurial-devel > <mercurial-devel@mercurial-scm.org> writes: > >> # HG changeset patch >> # User Martin von Zweigbergk <martinvonz@google.com> >> # Date 1497992441 25200 >> # Tue Jun 20 14:00:41 2017 -0700 >> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >> # Parent 0906385672d754f13a512fe70759f0463a069f1e >> py3: catch StopIteration from next() in generatorset >> >> IIUC, letting the StopIteration through would not cause any bugs, but >> not doing it makes the test-py3-commands.t pass. >> >> I have also diligently gone through all uses of next() in our code >> base. They either: >> >> * are not called from a generator >> * pass a default value to next() >> * catch StopException >> * work on infinite iterators >> * request a fixed number of items that matches the generated number >> * are about batching in wireproto which I didn't quite follow >> >> I'd appreciate if Augie or someone else could take a look at the >> wireproto batching and convince themselves that the next(batchable) >> calls there will not raise a StopIteration. > > I was just thinking of doing something like this. Shouldn't we just > 'return None' in Mercurial instead of 'raise StopIteration'? I thought "return" was just short for "return None". Are you just saying that we prefer "return None" in Mercurial? Should I send a v2?
On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk <martinvonz@google.com> wrote: > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >> Martin von Zweigbergk via Mercurial-devel >> <mercurial-devel@mercurial-scm.org> writes: >> >>> # HG changeset patch >>> # User Martin von Zweigbergk <martinvonz@google.com> >>> # Date 1497992441 25200 >>> # Tue Jun 20 14:00:41 2017 -0700 >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >>> # Parent 0906385672d754f13a512fe70759f0463a069f1e >>> py3: catch StopIteration from next() in generatorset >>> >>> IIUC, letting the StopIteration through would not cause any bugs, but >>> not doing it makes the test-py3-commands.t pass. >>> >>> I have also diligently gone through all uses of next() in our code >>> base. They either: >>> >>> * are not called from a generator >>> * pass a default value to next() >>> * catch StopException >>> * work on infinite iterators >>> * request a fixed number of items that matches the generated number >>> * are about batching in wireproto which I didn't quite follow >>> >>> I'd appreciate if Augie or someone else could take a look at the >>> wireproto batching and convince themselves that the next(batchable) >>> calls there will not raise a StopIteration. >> >> I was just thinking of doing something like this. Shouldn't we just >> 'return None' in Mercurial instead of 'raise StopIteration'? > > I thought "return" was just short for "return None". Are you just > saying that we prefer "return None" in Mercurial? Should I send a v2? Oh, never mind. You're of course talking about not using "raise StopIteration" in general, not specifically about this patch. Yes, that's probably true. I think there were just a few more cases of "raise StopIteration" to deal with. Anyway, that won't fix this case, because next() itself raise StopIteration.
Martin von Zweigbergk <martinvonz@google.com> writes: > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >> Martin von Zweigbergk via Mercurial-devel >> <mercurial-devel@mercurial-scm.org> writes: >> >>> # HG changeset patch >>> # User Martin von Zweigbergk <martinvonz@google.com> >>> # Date 1497992441 25200 >>> # Tue Jun 20 14:00:41 2017 -0700 >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >>> # Parent 0906385672d754f13a512fe70759f0463a069f1e >>> py3: catch StopIteration from next() in generatorset >>> >>> IIUC, letting the StopIteration through would not cause any bugs, but >>> not doing it makes the test-py3-commands.t pass. >>> >>> I have also diligently gone through all uses of next() in our code >>> base. They either: >>> >>> * are not called from a generator >>> * pass a default value to next() >>> * catch StopException >>> * work on infinite iterators >>> * request a fixed number of items that matches the generated number >>> * are about batching in wireproto which I didn't quite follow >>> >>> I'd appreciate if Augie or someone else could take a look at the >>> wireproto batching and convince themselves that the next(batchable) >>> calls there will not raise a StopIteration. >> >> I was just thinking of doing something like this. Shouldn't we just >> 'return None' in Mercurial instead of 'raise StopIteration'? > > I thought "return" was just short for "return None". Are you just > saying that we prefer "return None" in Mercurial? Should I send a v2? Sorry, no, I was just being sloppy ;-P I meant going through and replacing 'raise StopIteration' with 'return' (e.g. return None) and therefore we shouldn't need to catch StopIteration, correct? Here are the calls I found: ../mercurial/commandserver.py 142: raise StopIteration ../mercurial/manifest.py 124: raise StopIteration 146: raise StopIteration ../mercurial/patch.py 151: raise StopIteration
Martin von Zweigbergk <martinvonz@google.com> writes: > On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk > <martinvonz@google.com> wrote: >> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >>> Martin von Zweigbergk via Mercurial-devel >>> <mercurial-devel@mercurial-scm.org> writes: >>> >>>> # HG changeset patch >>>> # User Martin von Zweigbergk <martinvonz@google.com> >>>> # Date 1497992441 25200 >>>> # Tue Jun 20 14:00:41 2017 -0700 >>>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >>>> # Parent 0906385672d754f13a512fe70759f0463a069f1e >>>> py3: catch StopIteration from next() in generatorset >>>> >>>> IIUC, letting the StopIteration through would not cause any bugs, but >>>> not doing it makes the test-py3-commands.t pass. >>>> >>>> I have also diligently gone through all uses of next() in our code >>>> base. They either: >>>> >>>> * are not called from a generator >>>> * pass a default value to next() >>>> * catch StopException >>>> * work on infinite iterators >>>> * request a fixed number of items that matches the generated number >>>> * are about batching in wireproto which I didn't quite follow >>>> >>>> I'd appreciate if Augie or someone else could take a look at the >>>> wireproto batching and convince themselves that the next(batchable) >>>> calls there will not raise a StopIteration. >>> >>> I was just thinking of doing something like this. Shouldn't we just >>> 'return None' in Mercurial instead of 'raise StopIteration'? >> >> I thought "return" was just short for "return None". Are you just >> saying that we prefer "return None" in Mercurial? Should I send a v2? > > Oh, never mind. You're of course talking about not using "raise > StopIteration" in general, not specifically about this patch. Yes, > that's probably true. I think there were just a few more cases of > "raise StopIteration" to deal with. > > Anyway, that won't fix this case, because next() itself raise StopIteration. Seems I've had two race conditions today. Ah, I see. I'll be quiet now :-)
On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk via Mercurial-devel < mercurial-devel@mercurial-scm.org> wrote: > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: > > Martin von Zweigbergk via Mercurial-devel > > <mercurial-devel@mercurial-scm.org> writes: > > > >> # HG changeset patch > >> # User Martin von Zweigbergk <martinvonz@google.com> > >> # Date 1497992441 25200 > >> # Tue Jun 20 14:00:41 2017 -0700 > >> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 > >> # Parent 0906385672d754f13a512fe70759f0463a069f1e > >> py3: catch StopIteration from next() in generatorset > >> > >> IIUC, letting the StopIteration through would not cause any bugs, but > >> not doing it makes the test-py3-commands.t pass. > >> > >> I have also diligently gone through all uses of next() in our code > >> base. They either: > >> > >> * are not called from a generator > >> * pass a default value to next() > >> * catch StopException > >> * work on infinite iterators > >> * request a fixed number of items that matches the generated number > >> * are about batching in wireproto which I didn't quite follow > >> > >> I'd appreciate if Augie or someone else could take a look at the > >> wireproto batching and convince themselves that the next(batchable) > >> calls there will not raise a StopIteration. > > > > I was just thinking of doing something like this. Shouldn't we just > > 'return None' in Mercurial instead of 'raise StopIteration'? > > I thought "return" was just short for "return None". Are you just > saying that we prefer "return None" in Mercurial? Should I send a v2? > I'm not sure what the Python language itself specifies, but from the CPython API, you need to return a PyObject from every function and NULL is interpreted as an exception has been raised. If you have nothing to return, you Py_RETURN_NONE to return None as a PyObject. Yes, the return value of "return" is None. However, "return" and "return None" aren't semantically identical. For generators (functions containing "yield"), it is illegal to have "yield" and "return x" in the same function: you can only use bareword "return" for control flow purposes. So for this patch, "return None" will raise SyntaxError since the function has "yield."
On Tue, Jun 20, 2017 at 8:38 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote: > On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk via Mercurial-devel > <mercurial-devel@mercurial-scm.org> wrote: >> >> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >> > Martin von Zweigbergk via Mercurial-devel >> > <mercurial-devel@mercurial-scm.org> writes: >> > >> >> # HG changeset patch >> >> # User Martin von Zweigbergk <martinvonz@google.com> >> >> # Date 1497992441 25200 >> >> # Tue Jun 20 14:00:41 2017 -0700 >> >> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >> >> # Parent 0906385672d754f13a512fe70759f0463a069f1e >> >> py3: catch StopIteration from next() in generatorset >> >> >> >> IIUC, letting the StopIteration through would not cause any bugs, but >> >> not doing it makes the test-py3-commands.t pass. >> >> >> >> I have also diligently gone through all uses of next() in our code >> >> base. They either: >> >> >> >> * are not called from a generator >> >> * pass a default value to next() >> >> * catch StopException >> >> * work on infinite iterators >> >> * request a fixed number of items that matches the generated number >> >> * are about batching in wireproto which I didn't quite follow >> >> >> >> I'd appreciate if Augie or someone else could take a look at the >> >> wireproto batching and convince themselves that the next(batchable) >> >> calls there will not raise a StopIteration. >> > >> > I was just thinking of doing something like this. Shouldn't we just >> > 'return None' in Mercurial instead of 'raise StopIteration'? >> >> I thought "return" was just short for "return None". Are you just >> saying that we prefer "return None" in Mercurial? Should I send a v2? > > > I'm not sure what the Python language itself specifies, but from the CPython > API, you need to return a PyObject from every function and NULL is > interpreted as an exception has been raised. If you have nothing to return, > you Py_RETURN_NONE to return None as a PyObject. > > Yes, the return value of "return" is None. However, "return" and "return > None" aren't semantically identical. For generators (functions containing > "yield"), it is illegal to have "yield" and "return x" in the same function: > you can only use bareword "return" for control flow purposes. So for this > patch, "return None" will raise SyntaxError since the function has "yield." So, in summary, you both think the patch is good? If so, could one of you queue it? :-)
On Tue, Jun 20, 2017 at 11:09 PM, Martin von Zweigbergk <martinvonz@google.com> wrote: > On Tue, Jun 20, 2017 at 8:38 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote: >> On Tue, Jun 20, 2017 at 3:24 PM, Martin von Zweigbergk via Mercurial-devel >> <mercurial-devel@mercurial-scm.org> wrote: >>> >>> On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >>> > Martin von Zweigbergk via Mercurial-devel >>> > <mercurial-devel@mercurial-scm.org> writes: >>> > >>> >> # HG changeset patch >>> >> # User Martin von Zweigbergk <martinvonz@google.com> >>> >> # Date 1497992441 25200 >>> >> # Tue Jun 20 14:00:41 2017 -0700 >>> >> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >>> >> # Parent 0906385672d754f13a512fe70759f0463a069f1e >>> >> py3: catch StopIteration from next() in generatorset >>> >> >>> >> IIUC, letting the StopIteration through would not cause any bugs, but >>> >> not doing it makes the test-py3-commands.t pass. >>> >> >>> >> I have also diligently gone through all uses of next() in our code >>> >> base. They either: >>> >> >>> >> * are not called from a generator >>> >> * pass a default value to next() >>> >> * catch StopException >>> >> * work on infinite iterators >>> >> * request a fixed number of items that matches the generated number >>> >> * are about batching in wireproto which I didn't quite follow >>> >> >>> >> I'd appreciate if Augie or someone else could take a look at the >>> >> wireproto batching and convince themselves that the next(batchable) >>> >> calls there will not raise a StopIteration. >>> > >>> > I was just thinking of doing something like this. Shouldn't we just >>> > 'return None' in Mercurial instead of 'raise StopIteration'? >>> >>> I thought "return" was just short for "return None". Are you just >>> saying that we prefer "return None" in Mercurial? Should I send a v2? >> >> >> I'm not sure what the Python language itself specifies, but from the CPython >> API, you need to return a PyObject from every function and NULL is >> interpreted as an exception has been raised. If you have nothing to return, >> you Py_RETURN_NONE to return None as a PyObject. >> >> Yes, the return value of "return" is None. However, "return" and "return >> None" aren't semantically identical. For generators (functions containing >> "yield"), it is illegal to have "yield" and "return x" in the same function: >> you can only use bareword "return" for control flow purposes. So for this >> patch, "return None" will raise SyntaxError since the function has "yield." > > So, in summary, you both think the patch is good? If so, could one of > you queue it? :-) Sorry, I realized after sending that that may have sounded pushy. I also realize you may simply not have had time. I just meant to check if either of you still head reservations about the patch (potential wireproto changes is out of scope for this patch, IMO).
On Tue, 20 Jun 2017 14:45:57 -0700, Martin von Zweigbergk via Mercurial-devel wrote: > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com> > # Date 1497992441 25200 > # Tue Jun 20 14:00:41 2017 -0700 > # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 > # Parent 0906385672d754f13a512fe70759f0463a069f1e > py3: catch StopIteration from next() in generatorset Looks good as this _next() exists inside a generator function. Queued, thanks.
On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote: > Martin von Zweigbergk <martinvonz@google.com> writes: > > > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: > >> Martin von Zweigbergk via Mercurial-devel > >> <mercurial-devel@mercurial-scm.org> writes: > >> > >>> # HG changeset patch > >>> # User Martin von Zweigbergk <martinvonz@google.com> > >>> # Date 1497992441 25200 > >>> # Tue Jun 20 14:00:41 2017 -0700 > >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 > >>> # Parent 0906385672d754f13a512fe70759f0463a069f1e > >>> py3: catch StopIteration from next() in generatorset > >>> > >>> IIUC, letting the StopIteration through would not cause any bugs, but > >>> not doing it makes the test-py3-commands.t pass. > >>> > >>> I have also diligently gone through all uses of next() in our code > >>> base. They either: > >>> > >>> * are not called from a generator > >>> * pass a default value to next() > >>> * catch StopException > >>> * work on infinite iterators > >>> * request a fixed number of items that matches the generated number > >>> * are about batching in wireproto which I didn't quite follow > >>> > >>> I'd appreciate if Augie or someone else could take a look at the > >>> wireproto batching and convince themselves that the next(batchable) > >>> calls there will not raise a StopIteration. > >> > >> I was just thinking of doing something like this. Shouldn't we just > >> 'return None' in Mercurial instead of 'raise StopIteration'? > > > > I thought "return" was just short for "return None". Are you just > > saying that we prefer "return None" in Mercurial? Should I send a v2? > > Sorry, no, I was just being sloppy ;-P I meant going through and > replacing 'raise StopIteration' with 'return' (e.g. return None) and > therefore we shouldn't need to catch StopIteration, correct? Here are > the calls I found: > > ../mercurial/commandserver.py > 142: raise StopIteration > > ../mercurial/manifest.py > 124: raise StopIteration > 146: raise StopIteration > > ../mercurial/patch.py > 151: raise StopIteration IIUC, these StopIteration are valid since they are iterator/generator types. On the other hand, generatorset._iterator.gen() is a generator function, which should just end (without raising StopIteration) when all items are yielded.
Yuya Nishihara <yuya@tcha.org> writes: > On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote: >> Martin von Zweigbergk <martinvonz@google.com> writes: >> >> > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >> >> Martin von Zweigbergk via Mercurial-devel >> >> <mercurial-devel@mercurial-scm.org> writes: >> >> >> >>> # HG changeset patch >> >>> # User Martin von Zweigbergk <martinvonz@google.com> >> >>> # Date 1497992441 25200 >> >>> # Tue Jun 20 14:00:41 2017 -0700 >> >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >> >>> # Parent 0906385672d754f13a512fe70759f0463a069f1e >> >>> py3: catch StopIteration from next() in generatorset >> >>> >> >>> IIUC, letting the StopIteration through would not cause any bugs, but >> >>> not doing it makes the test-py3-commands.t pass. >> >>> >> >>> I have also diligently gone through all uses of next() in our code >> >>> base. They either: >> >>> >> >>> * are not called from a generator >> >>> * pass a default value to next() >> >>> * catch StopException >> >>> * work on infinite iterators >> >>> * request a fixed number of items that matches the generated number >> >>> * are about batching in wireproto which I didn't quite follow >> >>> >> >>> I'd appreciate if Augie or someone else could take a look at the >> >>> wireproto batching and convince themselves that the next(batchable) >> >>> calls there will not raise a StopIteration. >> >> >> >> I was just thinking of doing something like this. Shouldn't we just >> >> 'return None' in Mercurial instead of 'raise StopIteration'? >> > >> > I thought "return" was just short for "return None". Are you just >> > saying that we prefer "return None" in Mercurial? Should I send a v2? >> >> Sorry, no, I was just being sloppy ;-P I meant going through and >> replacing 'raise StopIteration' with 'return' (e.g. return None) and >> therefore we shouldn't need to catch StopIteration, correct? Here are >> the calls I found: >> >> ../mercurial/commandserver.py >> 142: raise StopIteration >> >> ../mercurial/manifest.py >> 124: raise StopIteration >> 146: raise StopIteration >> >> ../mercurial/patch.py >> 151: raise StopIteration > > IIUC, these StopIteration are valid since they are iterator/generator types. > On the other hand, generatorset._iterator.gen() is a generator function, which > should just end (without raising StopIteration) when all items are yielded. The reason I brought it up is that python3.6 now emits a warning about 'raise StopIteration' being deprecated: https://www.python.org/dev/peps/pep-0479/ I'll send an RFC series and see what the feedback is.
On Wed, Jun 21, 2017 at 10:35 AM, Sean Farley <sean@farley.io> wrote: > Yuya Nishihara <yuya@tcha.org> writes: > >> On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote: >>> Martin von Zweigbergk <martinvonz@google.com> writes: >>> >>> > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >>> >> Martin von Zweigbergk via Mercurial-devel >>> >> <mercurial-devel@mercurial-scm.org> writes: >>> >> >>> >>> # HG changeset patch >>> >>> # User Martin von Zweigbergk <martinvonz@google.com> >>> >>> # Date 1497992441 25200 >>> >>> # Tue Jun 20 14:00:41 2017 -0700 >>> >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >>> >>> # Parent 0906385672d754f13a512fe70759f0463a069f1e >>> >>> py3: catch StopIteration from next() in generatorset >>> >>> >>> >>> IIUC, letting the StopIteration through would not cause any bugs, but >>> >>> not doing it makes the test-py3-commands.t pass. >>> >>> >>> >>> I have also diligently gone through all uses of next() in our code >>> >>> base. They either: >>> >>> >>> >>> * are not called from a generator >>> >>> * pass a default value to next() >>> >>> * catch StopException >>> >>> * work on infinite iterators >>> >>> * request a fixed number of items that matches the generated number >>> >>> * are about batching in wireproto which I didn't quite follow >>> >>> >>> >>> I'd appreciate if Augie or someone else could take a look at the >>> >>> wireproto batching and convince themselves that the next(batchable) >>> >>> calls there will not raise a StopIteration. >>> >> >>> >> I was just thinking of doing something like this. Shouldn't we just >>> >> 'return None' in Mercurial instead of 'raise StopIteration'? >>> > >>> > I thought "return" was just short for "return None". Are you just >>> > saying that we prefer "return None" in Mercurial? Should I send a v2? >>> >>> Sorry, no, I was just being sloppy ;-P I meant going through and >>> replacing 'raise StopIteration' with 'return' (e.g. return None) and >>> therefore we shouldn't need to catch StopIteration, correct? Here are >>> the calls I found: >>> >>> ../mercurial/commandserver.py >>> 142: raise StopIteration >>> >>> ../mercurial/manifest.py >>> 124: raise StopIteration >>> 146: raise StopIteration >>> >>> ../mercurial/patch.py >>> 151: raise StopIteration >> >> IIUC, these StopIteration are valid since they are iterator/generator types. >> On the other hand, generatorset._iterator.gen() is a generator function, which >> should just end (without raising StopIteration) when all items are yielded. > > The reason I brought it up is that python3.6 now emits a warning about > 'raise StopIteration' being deprecated: > > https://www.python.org/dev/peps/pep-0479/ But that's *inside generators* and Yuya was saying that the remaining few cases in our code base are not inside generator (functions). Maybe I misunderstood, but my impression was that that would still be allowed. > > I'll send an RFC series and see what the feedback is.
Martin von Zweigbergk <martinvonz@google.com> writes: > On Wed, Jun 21, 2017 at 10:35 AM, Sean Farley <sean@farley.io> wrote: >> Yuya Nishihara <yuya@tcha.org> writes: >> >>> On Tue, 20 Jun 2017 15:27:56 -0700, Sean Farley wrote: >>>> Martin von Zweigbergk <martinvonz@google.com> writes: >>>> >>>> > On Tue, Jun 20, 2017 at 2:53 PM, Sean Farley <sean@farley.io> wrote: >>>> >> Martin von Zweigbergk via Mercurial-devel >>>> >> <mercurial-devel@mercurial-scm.org> writes: >>>> >> >>>> >>> # HG changeset patch >>>> >>> # User Martin von Zweigbergk <martinvonz@google.com> >>>> >>> # Date 1497992441 25200 >>>> >>> # Tue Jun 20 14:00:41 2017 -0700 >>>> >>> # Node ID f86d21c457209f5f5a139bcc808be33fbd930424 >>>> >>> # Parent 0906385672d754f13a512fe70759f0463a069f1e >>>> >>> py3: catch StopIteration from next() in generatorset >>>> >>> >>>> >>> IIUC, letting the StopIteration through would not cause any bugs, but >>>> >>> not doing it makes the test-py3-commands.t pass. >>>> >>> >>>> >>> I have also diligently gone through all uses of next() in our code >>>> >>> base. They either: >>>> >>> >>>> >>> * are not called from a generator >>>> >>> * pass a default value to next() >>>> >>> * catch StopException >>>> >>> * work on infinite iterators >>>> >>> * request a fixed number of items that matches the generated number >>>> >>> * are about batching in wireproto which I didn't quite follow >>>> >>> >>>> >>> I'd appreciate if Augie or someone else could take a look at the >>>> >>> wireproto batching and convince themselves that the next(batchable) >>>> >>> calls there will not raise a StopIteration. >>>> >> >>>> >> I was just thinking of doing something like this. Shouldn't we just >>>> >> 'return None' in Mercurial instead of 'raise StopIteration'? >>>> > >>>> > I thought "return" was just short for "return None". Are you just >>>> > saying that we prefer "return None" in Mercurial? Should I send a v2? >>>> >>>> Sorry, no, I was just being sloppy ;-P I meant going through and >>>> replacing 'raise StopIteration' with 'return' (e.g. return None) and >>>> therefore we shouldn't need to catch StopIteration, correct? Here are >>>> the calls I found: >>>> >>>> ../mercurial/commandserver.py >>>> 142: raise StopIteration >>>> >>>> ../mercurial/manifest.py >>>> 124: raise StopIteration >>>> 146: raise StopIteration >>>> >>>> ../mercurial/patch.py >>>> 151: raise StopIteration >>> >>> IIUC, these StopIteration are valid since they are iterator/generator types. >>> On the other hand, generatorset._iterator.gen() is a generator function, which >>> should just end (without raising StopIteration) when all items are yielded. >> >> The reason I brought it up is that python3.6 now emits a warning about >> 'raise StopIteration' being deprecated: >> >> https://www.python.org/dev/peps/pep-0479/ > > But that's *inside generators* and Yuya was saying that the remaining > few cases in our code base are not inside generator (functions). Maybe > I misunderstood, but my impression was that that would still be > allowed. Yeah, I was confused; sorry!
Patch
diff --git a/mercurial/smartset.py b/mercurial/smartset.py --- a/mercurial/smartset.py +++ b/mercurial/smartset.py @@ -871,7 +871,10 @@ if i < _len(genlist): yield genlist[i] else: - yield _next(nextgen) + try: + yield _next(nextgen) + except StopIteration: + return i += 1 return gen()