py_trees icon indicating copy to clipboard operation
py_trees copied to clipboard

Selector __init__ is incorrectly typed

Open jbcpollak opened this issue 3 years ago • 4 comments

Selector's init is currently (2.1.6) typed as:

    def __init__(self, name="Selector", memory=False, children=None):

but should match Sequence and Parallel:

    def __init__(
        self,
        name: str="Selector",
        memory: bool=True,
        children: typing.List[behaviour.Behaviour]=None
    ):

in the current state, you can to something like this:

py_trees.composites.Selector('MySelector', [... behaviours here]) and mypy will allow it, but the list I guess gets interpreted as the boolean and the Selector has no children.

Explicitly setting memory, or using children= as a named variable fixes the problem.

jbcpollak avatar Jun 02 '21 20:06 jbcpollak

seems my initial diagnosis is wrong and this in an inherent problem with using boolean positional arguments in python. It looks like this may be a solution once it lands:

https://github.com/python/mypy/pull/10557

jbcpollak avatar Jun 02 '21 21:06 jbcpollak

Aside from the boolean thing, these are examples of cases mypy missed when I ran it on the package (mypy -p py_trees). Not sure I understand why mypy missed them.

How are you catching these? The only thing I can guess at is perhaps because you're running it on the tests as well?

stonier avatar Jun 03 '21 13:06 stonier

Also - should fix the typing for children regardless. It should be typing.Optional[typing.List[behaviour.Behaviour]].

stonier avatar Jun 03 '21 13:06 stonier

How are you catching these? The only thing I can guess at is perhaps because you're running it on the tests as well?

two reasons: first, see the https://github.com/splintered-reality/py_trees/pull/338 PR, where I enabled mypy to run on tests as well. Second, when upgrading our own project to use the new release, a lot of things broke, and this was one of the tricky ones.

these are examples of cases mypy missed when I ran it on the package (mypy -p py_trees). Not sure I understand why mypy missed them.

its because mypy is a bit "soft" by default. I have a second branch I'll create a PR for in just a minute that will enable additional checks. The additional checks found many issues, which I also fixed.

jbcpollak avatar Jun 03 '21 15:06 jbcpollak

children: typing.Optional[typing.List[behaviour.Behaviour]] = None

Fixed in #380.

stonier avatar Jan 22 '23 06:01 stonier