-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Unexpected elements in usage #277
Comments
@elszben Interesting...
|
I am not sure I understand your request. I did give each argument exactly once. The command line arguments are these during the test: -c gd -d fdf Are you asking if it works if I supply the correct arguments? It seems to work in that case. |
@elszben I meant for |
@elszben Okay. So two issues here. Sorry. I mis-read something somewhere. Okay.
|
I don't have the time to patch this right now, quickly providing line numbers and suggestions for anyone with such time
|
Good catch @james-darkfox @elszben thanks for taking the time to file this, once it's fixed we'll report back. |
Once this issue is fixed we'll put out v1.4.3 |
fix(Help Message): required args no longer double list in usage Closes #277
v1.4.3 is out on crates.io which fixes this issue |
Thank you! I'll check it when I have time. |
I had time to check the solution and it does not solve all faults described in this bug report (at least not in all cases). The supplied arguments are correctly filtered out of the required args. The USAGE text is sometimes filtered and sometimes not. The original program invoked with -c gd -d fdf -g shows this output:
In this case the solution is not applied and the USAGE text still contains the supplied arguments twice. Another interesting bug is that if I call the program with a valid argument but "accidentally" concatenate the argument and its value then the error states that the -c argument is not valid. This is clearly wrong, the -c is a valid argument, it is even stated in the USAGE text.
|
Thanks for letting us know! I'll start re-investigating now. |
Great, thanks:) |
I've tried it and it works on all cases I've previously reported. However there are still cases when it does not:) I did not even try to understand how clap works internally but based on this bug report it seems to me that this functionality is copied a few times, maybe it would be worth to change that? Just an idea. Anyway, here is the not correct case:
|
@elszben This problem is due to the terribly organic way this library has been written. This makes testing fairly difficult as there are too many edge cases. I started a re-write in attempt to counter these types of problems. This re-write is being tracked in
No doubt about it. |
@elszben I apologize! We've been slowly refactoring away cases like this because it's like @james-darkfox said, edge cases pop up from everywhere! :) I'm going to make a quick change to this copy-paste-pasta which should eliminate this issue entirely as there will be only a single instance of determining what goes in the usage string...not the many that there are now. Once it's done, I'd be very interested if you find another case of duplication! Thanks again for taking the time to file and test these! 👍 |
No need to apologize, and there is certainly no need to hurry, I have all the time in the world:) I don't need a quick fix, just make it correct:) |
It mostly works but now it also crashes:) Example:
This is from v1.4.4 branch. p.s.: This is so funny, it just does not want to work:) |
I've added some assertions to ensure that doesn't happen again. Once #314 merges I'm going to ahead an put out 1.4.5 since it fixes a crash. @elszben Do you have your full test list? Just so we can ensure we're catching them all...and then we'll also add those to the actual tests 😄 Also, I'm leaving this open until we know for sure this issue is gone 😜 |
fix: fixes crash on invalid arg error Once this merges I'm going to ahead and put out 1.4.5 since this fixes a crash. Relates to #277
1.4.5 is out |
Unfortunately I do not have a list, originally I only had a single use case I wanted to be covered then I just came up with more cases that broke clap. I believe they are all included in this bug but here is a "complete" list:
|
Interesting "feature" in 1.4.5.
If an argument is provided twice and one of them is concatenated with its value then the argument name is printed with its value:) |
That's the intended functionality. In that particular use case (using It'd be possible for us to change it so that it only shows the argument, without the value, but I'd imagine that's somewhat bike shedding as the main goal is to alert the user to an error and where to find the error. 😄 |
Well, it is up to you, it does not hurt the usability in my use case:) I consider this bug to be fixed then. Thanks for all the help:) |
This program with arguments -c gd -d fdf generates a really weird, unexpected error message:
The argument c and d were supplied but the program lists them as not supplied. I think this is plain wrong but I don't know what the design was, let's say that this is working as intended.
However the USAGE text lists -c and -d twice and I can't come up with a situation where this is actually useful or makes sense.
I've tried to figure out why the second fault happens and added some debug code into clap. It seems that create_usage() creates a list of args from known required args and it just appends the current matches to the list but does not remove duplicates. No idea how to fix this elegantly while also keeping the order (is that even necessary?). This context specific usage generation seems weird to me:) The library is quite nice otherwise.
The text was updated successfully, but these errors were encountered: