Conversation
"The feature `bench_black_box` has been stable since 1.66.0 and no longer requires an attribute to enable"
This is definitely a nitpick and mostly my own opinion. "option requires a value: ..." makes it sound like the option requires itself "option does not require a value: ..." makes it sound like the value is optional Since the text will always be something the programmer has agreed is an acceptable option. It should be fine to just embed it in the middle of the message.
This emulates the behaviour of python's argparse when you specify options that look like negative numbers in your parser (e.g. -1 or -.5 or -123.456 which are not all short options but argparse is versatile). This is useful if you take a optionally negative number as a positional argument. The default is to keep the normal behaviour with the alternative optionally enabled by calling Options::allow_number_short_options(..., false)
This is an idea stolen from rust-lang/rust#120048. It's useful for the next commit.
Instead of letting you choose what type you want to get back from the API, just make it a `&str`. This means that options can only ever be something which can be turned into valid UTF-8. This sounds restrictive but for users of `&str as Argument` it makes no difference and for the few users of `&[u8] as Argument` it simplifies a lot of things. It also makes `&[u8]` usable on windows via `OsStr::as_encoded_bytes` and makes `&OsStr as Argument` a possibility.
Implement `&OsStr as Argument` on top of `&[u8] as Argument`. The unsafe code to convert back from the sliced `&[u8]` keeps to the safety invariants defined for the `OsStr::from_encoded_bytes_unchecked` function.
|
Interesting! I have thought about changing the API before, mostly thinking about refactoring the code to mimic what Also, currently, the I haven't looked at your code yet, since I'm currently quite busy with university, but I'll try to get to it soon. Thanks for your contribution!! |
This is an interesting idea. I will try to look into incorporating it as an alternative branch at some point in the near future.
Yes I noticed this, but after working with the code a bit I realised that neither order made much sense. But your suggestion above would make this easier.
No worries, take your time. I will add comments if I make any changes/updates but I do not expect you to review anything. I really published this more as a discussion piece at this point in time. |
I ran valgrind on an epaper tablet to optimize the ever living fuck out of this crate. It is possible to make the improvements you've mentioned if you give up that goal of performance. Also to address a few of your commit messages, ae2bb9e: I'll give you the second one, 'expect' is indeed better. The first one was due to an assumption that others would also perceive 'option requires a value' as simply the enum variant rather than prose. I was coding for myself at the time and wasn't thinking about serving other neurotypes as users 5d3f623: starts delving more into behaviors that I would never personally need but that may be useful to serve users who think differently. As for the rest, especially 574e253, This breaks certain fundamental assertions of the crate such as not ever requiring or assuming UTF-8. For me those assertions are important, but they may not be as important for everyone. Thank you for putting together this PR, it is well made. |
|
5d3f623 is a bit weird of a feature, but I think it might make more sense if I confess that I was trying to port some code from python's argparse and discovered that my original code was relying on this specific weird feature.
So I think it's a fair complaint, for me it was a trade-off worth having. Although it's still worth noting that the patches never prevent end-users of the application from passing arbitrary byte sequences as option values or positional arguments. Only the option characters/names are forced into UTF-8. Thank you for the comments though, I appreciate the review. |
|
Ah, trying to use Rust to replace Python is an interesting use-case that I definitely did not consider. That is fair. |
3 participants
I would like to preface this by saying that I am happy to just create a fork of getargs with my ideas and leave this project be.
I have prepared a set of patches of progressive improvements which culminate in the addition of
&OsStr as Argumentsupport.I think more tests would be useful to have etc. But this was mainly a proof of concept for myself.
Additionally the patch set adds the feature of not negative numbers as options taken from argparse in python. This is optional.
Really, I am mostly interested in comments and feedback from others who have worked on this project as I think they may be best suited to evaluate the commits.
I am curious as to what I could have done better and if this could ever be merged.
Edit: It's also worth mentioning that this change does introduce some minor (10%-50%) performance regressions. For me this is acceptable given the performance is still above and beyond anything which I would ever be concerned by. But I would be curious to know if this can be improved on.