Opened 18 months ago

Closed 17 months ago

Last modified 17 months ago

#6302 enhancement closed fixed (fixed)

FlagConstant objects should be iterable

Reported by: wsanchez Owned by: wsanchez
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/iterable-flags-6302
(diff, github, buildbot, log)
Author: wsanchez Launchpad Bug:

Description

I have a method that takes a FlagConstant argument, and I want to iterate through the flags to (a) flip the switches the flags should flip and (b) make sure there's no flag being sent that I don't know how to support.

For example, the caller starts by composing a FlagConstant from others:

flags = QueryFlags.NOT | QueryFlags.caseInsensitive

This is passed to my method. What I'd like to make work is:

for flag in flags:
   ... # I get (QueryFlags.NOT, QueryFlags.caseInsensitive)

The best I can do for now, appears to be:

for flag in (flags._container.lookupByName(name) for name in flags.names):
  ...

Note that I had to use the private flags._container attribute above.

Change History (14)

comment:1 Changed 18 months ago by wsanchez

  • Summary changed from FlagsConstant objects should be iterable to FlagConstant objects should be iterable

comment:2 Changed 18 months ago by wsanchez

  • Author set to wsanchez
  • Branch set to branches/iterable-flags-6302

(In [37138]) Branch for #6302

comment:3 Changed 18 months ago by wsanchez

  • Keywords review added

Fixed in r37140

comment:4 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to wsanchez

Thanks for your contribution.

  1. Can you update the howto to document this, and provide an example of usage.
    • I don't doubt that this is useful, but I'm not sure when I would use this, and thus I can evaluate whether this API is the appropriate way to provide this functionality.
  2. The docstring for __iter__ could be improved.
    1. It lacks @return annotation.
    2. It describes why the method exists, rather than what it does.
    3. vend is obscure and potentially confusing.
  3. What happens if there are no flags, or one flag set in a constant? The docstring for the existing test suggests that these might be interesting cases to test.

Please resubmit for review after addressing the above points.

comment:5 Changed 17 months ago by exarkun

Also, perhaps consider naming this API something other than __iter__ (as _ConstantsContainer has iterconstants rather than __iter__).

comment:6 Changed 17 months ago by wsanchez

  • Keywords review added

After talking to exarkun and glyph, we decided to stick with __iter__, add an accompanying __contains__ to test for whether a flag in in a flag set, and to add __nonzero__ so that an empty flags is false.

A motivator for __iter__ over iterflags() was that implementing __contains__ without a matching __iter__ is off. We also like the readability of Permissions.USER_READ in mode over Permissions.USER_READ in mode.iterflags().

The constants HOWTO doc is updated to show iteration of flags, and truth testing of flags. It also now shows boolean operations for completeness.

Tom's note about testing for no and single flags makes sense; that's fixed. Also improved the docstrings.

comment:7 Changed 17 months ago by wsanchez

…so this now also addresses #6303

comment:8 Changed 17 months ago by wsanchez

…also #6304

comment:9 Changed 17 months ago by wsanchez

  • Owner wsanchez deleted

comment:10 follow-up: Changed 17 months ago by therve

  • Keywords review removed
  • Owner set to wsanchez
  • +        self.assertEqual(
    +            set(self.FXF.WRITE & self.FXF.READ), # No flags
    +            set(())
    +        )
    +        self.assertEqual(
    +            set(self.FXF.WRITE),
    +            set((self.FXF.WRITE,))
    +        )
    +        self.assertEqual(
    +            set(self.FXF.WRITE | self.FXF.EXCLUSIVE),
    +            set((self.FXF.WRITE, self.FXF.EXCLUSIVE))
    +        )
    

Please put the closing parenthesis on the same line as the second element.

  • +        self.assertTrue(self.FXF.WRITE in flags)
    +        self.assertFalse(self.FXF.READ in flags)
    

You can use assertIn/assertNotIn.

Looks great, +1.

comment:11 Changed 17 months ago by wsanchez

  • Resolution set to fixed
  • Status changed from new to closed

(In [37739]) Merge branches/iterable-flags-6302: iterable, truth-testable FlagConstant

Author: wsanchez
Reviewer: tom.prince
Fixes: #6302

Adds iter and nonzero to FlagConstant to decompose flags in a
flag set and make boolean treatment of a FlagConstant sane.

Update the Constants HOWTO.

comment:12 in reply to: ↑ 10 Changed 17 months ago by glyph

Replying to therve:

Please put the closing parenthesis on the same line as the second element.

This does not appear to be explicitly stated in either PEP8 or our coding standard. Do you have a reference? Some of the examples imply it; but they are all like

functionNameGoesHere(long, argument, list, like
                     this, ends, here)

On calendar server, we've been experimenting with a different style, where

shortFunc(arguments, that, require, line, breaks,
          wrap, then, vertically, align)

but also

someLongObjectName.andThenSomeLongMethodName().oopsOutOfCharacters(
    then, you, just, indent, by, four, spaces,
    without, vertical, alignment, plus, since,
    the, opening, paren, sits, alone,
    so, too, does, the, closing, one
)

I'd kinda like to use this style in Twisted as well. Thoughts?

comment:13 Changed 17 months ago by wsanchez

Specifically, in this case, the difference is:

self.assertEqual(
    set(self.FXF.WRITE | self.FXF.EXCLUSIVE),
    set((self.FXF.WRITE, self.FXF.EXCLUSIVE)))

versus:

self.assertEqual(
    set(self.FXF.WRITE | self.FXF.EXCLUSIVE),
    set((self.FXF.WRITE, self.FXF.EXCLUSIVE))
)

I find the latter to be far more readable, in that I can find the matching parens a lot more easily. In the former, looking at the last line you see set((…))) and have to note that the extra close paren is closing the self.asssertEqual(.

The indentation matches this common style in C:

for (…) {
    …
}

comment:14 Changed 17 months ago by therve

OK, personal preference and stuff. It has the advantage of yelling at you less when you do stuff like merging. I prefer the condensed version, but it's not enforced by pep8 or our coding standard indeed.

Note: See TracTickets for help on using tickets.