| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
IMHO, we should notice this feature on whatsnews for 3.10.
Sorry, something went wrong.
|
News entry added. |
Sorry, something went wrong.
|
this PR should have cited https://bugs.python.org/issue38250 |
Sorry, something went wrong.
| :class:`Flag` members can also be iterated over:: | ||
|
|
||
| >>> purple = Color.RED | Color.BLUE | ||
| >>> list(purple) | ||
| [<Color.BLUE: 2>, <Color.RED: 1>] |
There was a problem hiding this comment.
The meaning of "members" should be clarified. Specifically, the iteration intentionally omits aliases and compound members.
The tests should verify this.
I would describe it as "iterating over the bits of a flag value", because that's what it does. Using "bits" should be OK since Flag is documented as supporting bitwise operators.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree.
Sorry, something went wrong.
| def __iter__(self): | ||
| members, extra_flags = _decompose(self.__class__, self.value) | ||
| return (m for m in members if m._value_ != 0) | ||
|
|
There was a problem hiding this comment.
The _decompose function is heavyweight (fairly complex and includes sorting). I wouldn't expect this kind of overhead when iterating flag bits.
I can think of two efficient ways:
return (v for v in self.__class__ if v.value != 0 and (v.value & (v.value-1) == 0) and v in self)@staticmethod
def _bits(n):
while n:
b = n & (~n+1)
yield b
n ^= b
def __iter__(self):
return (self.__class__(v) for v in self._bits(self._value_))in fact _decompose is used in other places, like __invert__, making these operations really slow. Really the set of available bits should be available in a class-only attribute, like _all_.
Sorry, something went wrong.
There was a problem hiding this comment.
proposal (2) is 2x faster than the implementation in this PR, for an enum with only about 6 values...
>>> from enum import Flag, auto
>>> class Foo(Flag):
... NONE = 0
... A = auto()
... B = auto()
... C = auto()
... C2 = C
... AB = A | B
... D = auto()
... E = auto()
... F = auto()
...
>>> x = Foo.A | Foo.B | Foo.D | Foo.F
>>> import timeit
>>> timeit.timeit('list(x)', globals=locals(), number=10000)
0.6332780510000013proposal (2):
>>> timeit.timeit('list(x)', globals=locals(), number=10000)
0.36078684099999236
Sorry, something went wrong.
There was a problem hiding this comment.
I have become increasingly unhappy with the the multiple calls to _decompose. Your ideas (and others, too) are welcome.
Sorry, something went wrong.
There was a problem hiding this comment.
I have become increasingly unhappy with the the multiple calls to _decompose. Your ideas (and others, too) are welcome.
I do have ideas on it, and intended to open a separate issue for refactoring _decompose() and make a PR replacing it with something simpler, faster, and less buggy. (E.g. use the _bits() iterator; and I noticed that all callers of it either use the members or extra_flags output, never both; and the computation of extra_flags would be trivial if we had an __all__ attribute on the class.)
Regarding bugs, when it's used by repr() or str(), I don't think it's intended that a lone compound value is represented as-is, while a compound plus other value is represented as a redundant "compound plus underlying bits":
>>> class Foo(Flag):
... A = auto()
... B = auto()
... C = auto()
... BC = B | C
>>> repr(Foo.BC)
'<Foo.BC: 6>'
>>> repr(Foo.A | Foo.BC)
'<Foo.BC|C|B|A: 7>'
Sorry, something went wrong.
| def __iter__(self): | ||
| members, extra_flags = _decompose(self.__class__, self.value) | ||
| return (m for m in members if m._value_ != 0) | ||
|
|
There was a problem hiding this comment.
I think the use of __iter__() is problematic given that __contains__() has different semantics.
__contains__() returns true for members with value 0 or compound members, and __iter__() does not. It's bad form for these not to match.
>>> class Foo(Flag): ... NONE = 0 ... A = 1 ... B = 2 ... C = 4 ... C2 = C ... AB = A | B >>> x = Foo.A | Foo.B >>> list(x) [<Foo.B: 2>, <Foo.A: 1>] >>> Foo.NONE in x True >>> Foo.AB in x True
I'd argue that __contains__() is completely broken and no one wants that behavior. But assuming that can't be changed, using __iter__() to iterate the bits is probably not a good idea.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree that the zero-value flag should not be contained in a non-zero flag, but I have no problem with a compound flag showing up if its bits are present. You are welcome to try and convince me otherwise, but it will take more than "no one wants that behavior".
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, I just learned that Flag.__contains__() is technically a subset operator:
>>> class Foo(Flag):
... NONE = 0
... A = 1
... B = 2
... C = 4
... AB = A | B
>>> Foo.AB in (Foo.AB | Foo.C)
True
>>> (Foo.A|Foo.C) in (Foo.AB | Foo.C)
TrueIf that's the case, then actually it's OK for Foo.None in Foo.A to be True, since Foo.None is the empty set:
>>> set().issubset({1, 2})
TrueI would not have made the API this way. __contains__ should be strictly for testing whether a bit is in the value (a Flag value is a set of enabled bits, in my mind), while issubset() and operator <= should be used for subset.
Sorry, something went wrong.
|
also, since _decompose() itself has a bug that I noticed while reading it, the new iterator is buggy _decompose() has incorrect output when the value input matches multiple compound values, so __iter__ incorrectly includes a compound value: >>> class Foo(Flag):
... NONE = 0
... A = auto()
... B = auto()
... C = auto()
... AB = A | B
... ABC = A | B | C
>>> list(Foo.ABC)
[<Foo.C: 4>, <Foo.AB: 3>, <Foo.B: 2>, <Foo.A: 1>] |
Sorry, something went wrong.
|
Yeah, I had recently noticed that myself. Only the one-bit flags should be in the iteration. |
Sorry, something went wrong.
|
I'm not sure why Flag allows definitions that cannot be decomposed into single-bit members. >>> class Bizarre(Flag):
... b = 3
... c = 4
... d = 6
...
Bizarre
>>> list(Bizarre(7))
[<Bizarre.d: 6>, <Bizarre.c: 4>, <Bizarre.b: 3>]The iterator is no longer enumerating bits, and this defies any kind of efficient implementation of Flags based on bits (like trivial decomposition and calculation of extra_args). I propose that Flag raise ValueError on such a definition. Supporting it is not worth the code complexity and efficiency cost. |
Sorry, something went wrong.
|
and this is nonsensical: >>> ~Bizarre.d
<Bizarre.0: 0>Other than making such definitions invalid, another approach is to use pseudo members to fill in missing bits. But they should be applied uniformly across the constructor, operations, and repr: >>> class Bizarre(Flag):
... b = 3
... c = 4
... d = 6
...
Bizarre
>>> list(Bizarre(7))
[<Bizarre.c: 4>, <Bizarre.2: 2>, <Bizarre.1: 1>]
>>> Bizarre(2)
<Bizarre.2: 2>
>>> Bizarre(8)
ValueError: 8 is not a valid Bizarre
>>> ~Bizarre.d
<Bizarre.1: 1> |
Sorry, something went wrong.
|
Flag handling of negative numbers is also dubious: >>> class Foo(Flag):
... X = 1
...
>>> Foo(1)
<Foo.X: 1>
>>> Foo(-1)
<Foo.X: 1>
>>> Foo(-2)
<Foo.0: 0>
>>> Foo(-3)
ValueError: -3 is not a valid FooI see the implementation in _missing_ (negative number is inverted and then inverted back after lookup)-- but what is the use case for this behavior? I don't think Flag should allow negative numbers. |
Sorry, something went wrong.
|
summary of Flag bugs to file:
|
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Member and member combinations can now be iterated over:
class Color(Flag): RED = 1 BLUE = 2 GREEN = 4 purple = Color.RED | Color.BLUE list(purple) # [<Color.BLUE: 2>, <Color.RED: 1>]https://bugs.python.org/issue32218