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

Support Option<Option<T>> field types #190

Merged
merged 6 commits into from
May 29, 2019

Conversation

sphynx
Copy link
Contributor

@sphynx sphynx commented May 28, 2019

Closes #188

I used sub_type function in Attrs module, so I moved it to there and exported so that it can be reused in lib.rs, I am not sure this is an optimal solution, please let me know if you want to change that (for example to have a separate module for types manipulations or anything else).

Another thing that I wanted to add to tests: is to make sure that Option<Option<T>> cannot be used for positional arguments. In this case I would expect a macro code-generation to fail, but I'm not sure how to make a test which expects particular macro failure. For now, I just checked that it works manually.

Also please note that I am using max_values=1 as well as min_values=0 to limit number of values to 1 in the generated clap code. However, handling of max_option in clap apparently has a bug: it tries to consume one more argument than it should during the parsing.

I fixed that bug and prepared a PR here: clap-rs/clap#1481
However, before that bug is fixed, theoretically we can have problems if, say, Option<Option<T>> long field with a value is immediately followed by a subcommand, like this:

test --field 1 add -p 2

then clap (without my fix) would continue consuming arguments for --field even when max_values=1 is set, so it will consume subcommand add as well and parsing will fail on seeing -p. But this is not directly related to Option<Option<T>> feature, just something to keep mind since it's related to max_values.

Please let me know if you have any suggestions about the code.

@TeXitoi
Copy link
Owner

TeXitoi commented May 29, 2019

you need to rustfmt please.

Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Only a minor stylistic remark.

structopt-derive/src/attrs.rs Show resolved Hide resolved
@TeXitoi
Copy link
Owner

TeXitoi commented May 29, 2019

No problem for the sub_type move.

There is no (yet) compile failure tests, so I don't ask to do this. But I'll appreciate a dedicated PR adding this new infrastructure ;-)

Noted for the clap problem, can be useful if a related bug report rises.

@TeXitoi
Copy link
Owner

TeXitoi commented May 29, 2019

Also can you update the CHANGELOG.md? Thanks.

@TeXitoi
Copy link
Owner

TeXitoi commented May 29, 2019

I forgot a last thing: this feature must be documented.

@sphynx
Copy link
Contributor Author

sphynx commented May 29, 2019

I've fixed everything you mentioned.

Not sure about CHANGELOG format: should it be under a new version header? For now I've just added a new item on the very top without the version header. I can bump the package version as well (but perhaps you'd like to merge other pull requests first).

Thanks for your feedback!

@TeXitoi TeXitoi merged commit 118731b into TeXitoi:master May 29, 2019
@TeXitoi
Copy link
Owner

TeXitoi commented May 29, 2019

Thanks!

@TeXitoi
Copy link
Owner

TeXitoi commented May 29, 2019

v0.2.16 published

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 this pull request may close these issues.

Support optional arguments for options
2 participants