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

what is the purpose of special-casing u64? #30

Closed
rocallahan opened this Issue Nov 22, 2017 · 12 comments

Comments

Projects
None yet
7 participants
@rocallahan

rocallahan commented Nov 22, 2017

I want to have parameter values parsed into u64s, but they get special treatment instead. Apparently a u64 captures all remaining parameters and is set to the number of remaining parameters? How is that useful if the values aren't accessible? Wouldn't it make more sense for users to just use a Vec<String> here and call len() when they want the number of parameters?

@TeXitoi

This comment has been minimized.

Owner

TeXitoi commented Nov 22, 2017

That's a very common pattern. You find it a lot with verbose option. -v for verbose, -vv for very verbose, -vvv for extremely verbose. That would be inconvenient to have to call .len() on a vector.

If you want a u64, just do that:

#[structopt(short = "i", parse(try_from_str))]
integer: u64,

@TeXitoi TeXitoi closed this Nov 22, 2017

@rocallahan

This comment has been minimized.

rocallahan commented Nov 22, 2017

Ah, it wasn't clear to me from the documentation that "number of params" means the number of repetitions of that parameter, rather than the number of remaining parameters. Maybe that could be clarified.

And does it really make sense for it to be a u64? Why not a u8 or u16? Any command-line interface that expects the user to repeat a parameter more than 255 times seems like it must be poorly designed.

Anyway, thanks for structopt! I like it very much.

@TeXitoi

This comment has been minimized.

Owner

TeXitoi commented Nov 22, 2017

That's a u64 because the corresponding method in clap returns a u64.

I reopen the issue for the documentation clarification. I'll do it when I'll have the time. Feel free to open a PR if you are inspired.

Thanks for the feedback, that's always motivating to know that what we do is useful.

@TeXitoi TeXitoi reopened this Nov 22, 2017

@TeXitoi TeXitoi closed this in dca4b8d Nov 23, 2017

@HongbinZhou

This comment has been minimized.

HongbinZhou commented Nov 26, 2017

First thanks for structopt! I am new to it but I like it already.

When I read the doc I was also confused by the special handling of u64, at first glance the doc and examples seems not show how one can parse an integer parameter larger than max of u32 (if one really want to input a parameter larger than 2^32), as u64 is special for repetitions of the used arguments. I searched the issues and luckily I found this one, I'm glad I'm clear after reading the issue.

Maybe such info can be added to doc:

That's a very common pattern. You find it a lot with verbose option. -v for verbose, -vv for very verbose, -vvv for extremely verbose. That would be inconvenient to have to call .len() on a vector.

If you want a u64, just do that:

    #[structopt(short = "i", parse(try_from_str))]
    integer: u64,

@TeXitoi TeXitoi reopened this Nov 26, 2017

@TeXitoi

This comment has been minimized.

Owner

TeXitoi commented Nov 26, 2017

Looks like that's not yet clear enough. I'll try to improve that.

@TeXitoi TeXitoi added the doc label Nov 27, 2017

@m4b

This comment has been minimized.

m4b commented Dec 29, 2017

Wowwww this just bit me so hard. Considering that this is treated specially and switching from a u64 to a u32 completely changes the semantics of the command line parser i sincerely wish that you’d remove this, as it definitely violates the principle of least surprise in multiple ways, etc.

This is not a user friendly feature at all.

For something that drastically modifies the behavior like this it should definitely be typed, like adding a thin type wrapper like Repetition(u64).

Also repurposing an (arbitrary) type to do something like reputation of arguments, which quite frankly is not common at all (I’ve never done this and I’ve written I don’t know how many command line tools using this crate at this point), and which is very likely only ever used with something like verbosity (if it’s used at all) just seems odd.

If the underlying crate is to blame there is no reason to repeat that mistake, and is easily rectified by a type wrapper, etc.

@rocallahan

This comment has been minimized.

rocallahan commented Dec 29, 2017

I agree. I've written 18 tools using structopt and none of them count repeated arguments. Also I can't remember the last time I used repeated arguments in any command. A type wrapper or an attribute would work just as well and avoid this pitfall.

@aleksanb

This comment has been minimized.

aleksanb commented Jan 7, 2018

This was rather surprising, given that a field with the type u32 works the way you'd expect, while switching to u64 suddenly doesn't.

It's feels rather unintuitive. A solution using Repetition(u64) as suggested by m4b would probably be better.

@SergioBenitez

This comment has been minimized.

Contributor

SergioBenitez commented Jan 28, 2018

I, too, wasted time because of this. It's incredibly surprising, and I skipped right past it in the documentation because I really wouldn't have expected this behavior. I also recommend making u64 work as if it was parse(try_from_str) by default. To enable the functionality it currently has, you might consider recognizing a new field in variant attributes (say parse(multiple)) that works for any unsigned integer type. An alternative, as already proposed, is to recognize a custom type, say Repetition, but that would be the only type exported from StructOpt and again feels like an odd special case.

@TeXitoi

This comment has been minimized.

Owner

TeXitoi commented Jan 28, 2018

That will be parse(from_occurrences) and I've planned to do that for the 0.2. It will do a From::from(m.occurrences_of("name")) by default.

I can't find the time to do that, but I hope to do it soon.

@SergioBenitez

This comment has been minimized.

Contributor

SergioBenitez commented Jan 28, 2018

@TeXitoi No worries! I find value in structopt, and it was a breeze to implement, so I went ahead and submitted a PR (#48). I named it parse(multiple) as I hadn't seen your comment. Happy to rename it. It currently uses as to convert between integer types. From<u64> isn't implemented for any of the unsigned integer types except u64 and the unstable u128, so it doesn't seem like the right way to go.

@TeXitoi TeXitoi added enhancement v0.2 and removed doc labels Jan 29, 2018

@TeXitoi TeXitoi closed this in #48 Feb 3, 2018

@vi

This comment has been minimized.

vi commented Feb 6, 2018

Isn't using u64 for counting -vs an overkill on 32-bit platforms? Why not usize.

Anyway special-casing u64 here feels like special-casing std::vector<bool> in C++ - trading general consistency for little immediate use case.


I see this is already being addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment