NEW: Add functionality that can capture values set inside of an action in provenance

[Oddant1]

Close #823

[Oddant1]

Should we throw an error if a value was never set on the CaptureHolder?

[ebolyen]

I like that idea and it's easier to relax that rule if we had to.

I think so long as param.set_value(None) is legal than there's no real downside.

[Oddant1]

@ebolyen How do we feel about rules for sentinel values? Are we ok with making the assumption there will only be one sentinel value? Do we feel ok with making the assumption that sentinel value will be the default value for the param? Makes a difference to how we determine if a value was set on the capture

[ebolyen]

I see some of the issues here.

I like how simple the above code looks. Adding a rule about set_value would either mean you make defaults special to it, or have to do somewhat ugly things.

    if random_seed.value is None:
        random_int = random.randrange(sys.maxsize)
    else:
        random_int = random_seed.value

    random_seed.set_value(random_int)

This just doesn't flow very nicely. So if we want to avoid setting what has already been set, then we need the default value to be treated as un-set, which is pretty much how we handle None, but if it was a value like 3 it gets weird. Although arguably, if 3 was the default, then there's nothing much to capture in the first place.

Given all of that, I think maybe the best approach is the default must be None and we treat that as un-set. Then to determine if param.set_value() has been called, we use a simple flag instead of the value itself. This permits None to be the value, but it requires the plugin to decide that rather than happening on accident and let's you skip param.set_value if it is anything other than None.

So then our logic would look like:

def _is_set(self):
    return self._set_value_called or self.value is not None

[Oddant1]

Ok I did a buncha refactoring to follow these rules

[Oddant1]

If a value was passed into a capture parameter, do we want to treat that as the capture being set?

I think doing that would be nice, but the value they explicitly passed in may be a sentinel value for capturing. The default value for a Capture parameter may not be a sentinel, or it may not be the only sentinel.

[ebolyen]

Looks pretty good, but still need to pull it down and play with it.

One thing we'll need to do is add an import alias to qiime2.plugin. Also I think the actual implementation may want to be outside of core/type/primitive.py, but I don't actually have a nice place for it to live. Maybe a new core/arguments.py module?

[Oddant1]

but I don't actually have a nice place for it to live

@ebolyen Maybe it just goes in signature.py?

[ebolyen]

Maybe it just goes in signature.py?

Yeah that sounds fine to me.

qiime2/core/type/signature.py (1964 - Full Color)

[ebolyen]

LGTM, just two tiny comments. Feel free to merge when ready.