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

Should Optional[Tuple[type1]] be supported #115

Closed
neelmraman opened this issue Jun 11, 2022 · 7 comments · Fixed by #116
Closed

Should Optional[Tuple[type1]] be supported #115

neelmraman opened this issue Jun 11, 2022 · 7 comments · Fixed by #116

Comments

@neelmraman
Copy link
Contributor

I noticed that while Optional[SomeContainer[type1]] is supported for most of the containers defopt supports (i.e., List, Sequence, Collection, Iterable), it is not supported for Tuple. I would expect Optional[Tuple[type1]] to behave similarly to other containers, in that it is treated as simply Tuple[type1]. I was able to add support for this locally within a couple of minutes, but figured I would check to make sure it wasn't deliberately unsupported before pushing the fix.

And thanks again for actively maintaining defopt! I use it quite a bit in my job.

@anntzer
Copy link
Owner

anntzer commented Jun 12, 2022

Seems like an oversight, and I would (probably?) accept a PR fixing it. It should also support Tuple[...] | None (as that's basically the same thing), and should error out if there is a custom parser registered for NoneType (unless a 1-element tuple type is given), as in that case the arity (number of items to read) is ill-defined.

@neelmraman
Copy link
Contributor Author

and should error out if there is a custom parser registered for NoneType

Oh is this only for the optional tuple case? Or any optional container?

And do you mind providing an example of the ambiguity that a 1-element tuple avoids?

@anntzer
Copy link
Owner

anntzer commented Jun 14, 2022

Say you have a parser for NoneType (def parse_none(s): return s if s == ":none:" else raise ValueError("not none")) and a function with hint def main(*, opt: Optional[tuple[str, str]): .... Remember that this is equivalent to def main(*, opt: None | tuple[str, str]) (Optional is strictly equivalent to union-with-None, and defopt always special-cases None to go first in the union). Now, the problem is that the invocation $ myscript --opt <some-arg> is valid if and only if <some-arg> is :none:, and the invocation $ myscript --opt <some-arg> <other-arg> is valid if and only if <some-arg> is not :none:, i.e. nargs for opt is not well defined (and argparse has no way to represent such options (IIRC there's a bug on CPython's tracker to request support for that)). On the other hand, for 1-element tuples, there is no ambiguity that nargs == 1.

Your point about other optional containers makes me believe that I have indeed missed that problematic interaction previously; Optional[list[str]] behaves exactly like list[str]. On the other hand, in for non-tuple containers, I think we can have a way out because they already have nargs = "*" anyways and therefore we can just do add a case checking if a single arg was passed and, if so, try to pass it to the none-parser (if it exists). (For tuples, we can't rely on using nargs = "*", as we can have multiple positional tuple parameters.)

(PS: this discussion also applies to #116, of course.)

@neelmraman
Copy link
Contributor Author

Ahh got it, yeah I'll work on that when I get the chance

neelmraman added a commit to neelmraman/defopt that referenced this issue Jun 18, 2022
@neelmraman
Copy link
Contributor Author

So I was able to raise an error when an optional multiple item tuple with a custom none parser is provided, but I'm a bit confused on where to make the change such that the none parser is tried first in the list/one-item tuple case. For optional primitives, trying the none parser first is more straightforward because that can happen via the type set on the ArgumentParser argument. To do this with optional containers, would that need to happen in _bind_or_bind_known somehow?

@anntzer
Copy link
Owner

anntzer commented Jun 18, 2022

From a quick look, this is indeed not trivial to implement :-( and would probably require a bit of rearchitecting. Let's go for the simpler route of erroring out in all cases when a custom noneparser is present, we can always consider adding back support for the one-element case later.

@neelmraman
Copy link
Contributor Author

Sure, sounds good. I imagine wanting to have an optional tuple potentially parsed as None via the command line (rather than just using a default value of None) is a pretty rare use case anyway.

neelmraman added a commit to neelmraman/defopt that referenced this issue Jun 18, 2022
anntzer added a commit that referenced this issue Jun 21, 2022
Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants