Andrew Barnert added the comment: > A Mercurial export patch should work with the Reitveld review thing if it is > a single revision (and based on a public revision).
Well, it's 7 separate commits on a work branch, so I could check in each piece and test it separately, and then I just compared the branch to its parent (default) for the export. But, as I said, I can flatten it into a single diff if the review system can't handle this way. Sorry for making the first patch hard to review, and thanks for reviewing it anyway; I'll fix that with the next one. > You have a stray print() call, probably in Hashable.__subclasshook__(). Thanks; fixed for the next patch. > For a the do-nothing __reversed__() implementation, do you think “yield from > ()” would be clearer than “while False”? Or even “return iter(())”? Maybe, but this way is identical to the definition for the do-nothing Iterable.__iter__. > What’s the advantage of having Reversed inherit Iterable? This was discussed briefly... somewhere, maybe the -ideas thread? Pro: If they were independent types, more people would probably register with/inherit from Reversible but not Iterable in error than intentionally. Pro: One less thing for Sequence to inherit from. This was mentioned in the context of adding both Reversible and HalfSequence (or whatever it ends up being called): "class Sequence(Iterable, Reversible, HalfSequence, Sized, Container) seems a little much; "class Sequence(Reversible, HalfSequence, Sized, Container)" is slightly better. Con: It's not completely impossible that a type could be reversible but not iterable. For example, you might have a reversible, and even indexable-with-negative-indicies-only, infinite collection of all the negative numbers. (But nobody could think of an actual _useful_ example of such a type.) Those are all pretty trivial points. When Guido said someone needs to work out where Reversible fits into the hierarchy, and there'd been enough talk without a patch. So I looked at where the discussion seemed to be leaning, couldn't find any additional problems with it when I looked into it further, and did it that way. Guido said he liked it on the Reversible bug, so I did it the same way here. So, no strong arguments or even feelings from anyone; if you've got one, definitely chime in. > How does the subclass hook interact with classes that implement > __reversed__() but not __iter__()? I don’t see how the > self.assertFalse(issubclass(K, Reversible)) test could pass. Exactly the same way Iterator's handles a class that defines __next__ but not __iter__. I'll double-check that it actually is implemented right before the next patch (and verify that the tests run), but it should be "return _check_methods(C, "__reversed__", "__iter__"). > Should the new Reversed class be excluded from collections.__all__, or is it > not worth it? It's Reversible, not Reversed. It's in __all__, and I'm pretty sure it should be there--if it's not worth exporting, it's not worth creating in the first place, or documenting, right? > I find the lambda generator a bit quirky in test_Reversible(). Maybe it would > be better as an ordinary non-lambda generator with a yield statement. This is taken from an identical test in test_Iterable, in the same file. > It would be good to have a versionadded notice for Reversible. Definitely; thanks for the catch. I'll add that for the next version of the patch. > I think there should also be one for allowing None for all special methods. I'm assuming you mean in collections.abc, not in the data model, right? The problem is where to put it. The collections.abc docs don't even mention that some of the ABCs have subclass hooks that detect "implicit subclasses", much less tell you which ones do and don't, much less tell you which among those that do treat None specially. Until we add that documentation, there's really nothing to contrast with. Maybe this means we need another bug where we rewrite the collections.abc docs to include all that information, and in the new documentation we can note what prior versions of Python did (or maybe prior versions of CPython--I doubt any major implementations were different, but without checking them all I wouldn't want to guarantee that)? > Instead of self.assertEqual(list(reversed(R())), list(reversed([]))), why not > check it is equal to an empty list directly? This might be a case of following the parallelism with test_Iterable too far; I can change it. > In test_contains.py, I would either write lambda: 0 in bc, or use the “with > self.assertRaises()” form. That makes sense; I'll do it. > Finally, if setting a binary operator method to None means that operation is > not available, I find it a bit surprising that doing this prevent the other > operand’s method from being tried. I guess you don’t want to change the > implementation, but perhaps the documentation of the binary operator methods > could be clarified. I definitely don't want to change the implementation unless there's a very good reason. Also, if you think it through, from either angle, the way it's always worked makes perfect sense. None (in fact, anything that raises a TypeError when called) always blocks fallback, so it blocks fallback to other.__radd__. Alternatively, the rules for when + looks for __radd__ are very specific and reasonably simple, and TypeError is not one of the things that triggers it. Making None trigger __radd__ would require making one of those two rules more complicated. And what would the benefit be? If you want to trigger fallback, there's already a simple, well-documented, readable way to do that; we don't need another one. And I think the note would be more likely to confuse someone who didn't need it (and maybe make him think he should be defining __add__ = None where he really shouldn't) than to help someone who did. Actual reasonable use cases for __add__ = None are rare enough that I think it's OK that someone has to understand what they're doing and be able to think through the rules and/or know what combinations to test via trial and error before they can use it. But maybe a footnote would work? For example, in the sentence "These functions are only called if the left operand does not support the corresponding operation and the operands are of different types. [2]", we could add another footnote after "does not support the corresponding operation", something like this: > [2] "Does not support" here means has no such method, or has a method that > returns NotImplemented, as described above. Do not assign the method to None > if you want to force fallback to the right operand's reflected method--that > will instead have the opposite effect of explicitly _blocking_ such fallback. Again, thanks for all the notes. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue25958> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
