required=false should be sufficient to make the whole Union property optional

[Yxwww]

@bsmithyman , @fwkoch

union_prop = properties.Union(
            'this is a union prop',
            props=(
                InstanceUID(
                    '',
                    Class_1,
                    required=False,  
                    ),
                InstanceUID(
                    '',
                    Class_2,
                    required=False,
                    )
                ),
            required=False,
            )

[bsmithyman]

Thanks @Yxwww
I was debugging this with Yuxi last night, and while it's functional I think we can improve this behaviour. Probably something like:

class Union(basic.Property):
    ...
    def validate(self, instance, value):
        """Check if value is a valid type of one of the Union props"""
        for prop in self.props:
            try:
                return prop.validate(instance, value)
            except (ValueError, KeyError, TypeError, AttributeError):
                continue
        if not self.required and (value is None or value is utils.undefined):
            return
        self.error(instance, value)

i.e., the last two lines before the self.error(...) call.

I think that would make it a bit cleaner. Thoughts @fwkoch?

[fwkoch]

Hey @Yxwww - Thanks for catching this! You are absolutely right on how required should work.

@bsmithyman - I think this needs to go in the assert_valid method. validate is used to say whether or not an assigned value is ok - it doesn't care about required or not. assert_valid happens on HasProperties.validate which does care about what is required vs. optional. I added similar handling to how container properties deal with this: #199

I'd like to give slightly more thought to some of the implications on this - eg what if the Union property is required, but one of it's props specifies required=False...? This may cause trouble. But the basics are already present in the PR.

[fwkoch]

Closed by #199