Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep names attribute and objects attribute of an ObjectSelector in sync #309

Closed
sdc50 opened this issue Dec 14, 2018 · 2 comments · Fixed by #598
Closed

Keep names attribute and objects attribute of an ObjectSelector in sync #309

sdc50 opened this issue Dec 14, 2018 · 2 comments · Fixed by #598
Labels
component: type/value stuff system of parameter type/value checking, inheritnace, etc etc status: discussion Discussion. Not yet a specific feature/bug. Likely to result in multiple PRs/issues. type-bug Bug report
Milestone

Comments

@sdc50
Copy link

sdc50 commented Dec 14, 2018

Now that the ObjectSelector accepts a mapping as the objects argument of the constructor it would be nice to also allow the objects attribute of the class to be set with a mapping. Otherwise, if the objects attribute is changed after instantiation then the names attribute is out of sync. For example:

image

One suggestion would be to make objects a property that has a setter:

class ObjectSelector(Selector):
...
     __slots__ = ['_objects','compute_default_fn','check_on_set','names']

    @property
    def objects(self):
        return self._objects

    @objects.setter
    def objects(self, objects):
        if objects is None:
            objects = []
        if isinstance(objects, collections.Mapping):
            self.names = objects
            self._objects = list(objects.values())
        else:
            self.names = None
            self._objects = objects

    # ObjectSelector is usually used to allow selection from a list of
    # existing objects, therefore instantiate is False by default.
    def __init__(self,default=None,objects=None,instantiate=False,
                 compute_default_fn=None,check_on_set=None,allow_None=None,**params):
        self.objects = objects
        ...

Then the behavior would be like I would expect:
image

@philippjfr
Copy link
Member

This should be fixed before we release 1.9.

@sdc50
Copy link
Author

sdc50 commented Dec 14, 2018

I'm not sure what the behavior should be when check_on_set is False and the property is set to a value that is not in objects. Perhaps the _ensure_value_is_in_objects method should update the names attribute. Maybe something like this would work:

    def _ensure_value_is_in_objects(self,val):
        """
        Make sure that the provided value is present on the objects list.
        Subclasses can override if they support multiple items on a list,
        to check each item instead.
        """

        if not (val in self.objects):
            self.objects.append(val)
            if self.names is not None:
                self.names[hashable(val)] = val

@ceball ceball added component: type/value stuff system of parameter type/value checking, inheritnace, etc etc status: discussion Discussion. Not yet a specific feature/bug. Likely to result in multiple PRs/issues. type-bug Bug report labels Apr 13, 2020
@tonyfast tonyfast added this to the v1.10.0 milestone Aug 24, 2020
@jbednar jbednar modified the milestones: v1.10.2, v1.10.3 Jun 30, 2021
@MridulS MridulS modified the milestones: v1.11.2, v1.12.1 Feb 7, 2022
@philippjfr philippjfr modified the milestones: v1.12.x, 2.0 Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: type/value stuff system of parameter type/value checking, inheritnace, etc etc status: discussion Discussion. Not yet a specific feature/bug. Likely to result in multiple PRs/issues. type-bug Bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants