-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
setting: IgnoreErrors - Allow parsing despite missing option values #2480
Conversation
03b108a
to
521183c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A really good start. Sorry for the late review. Was a little busy IRL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update on this?
Hi @pksunkara, thank you so much for getting back to me! I'll have time again to work on this during the weekend (a little bit on Saturday, more on Sunday, CEST time zone).Thank you for your patience :) |
521183c
to
dc35409
Compare
Hi @pksunkara, unfortunately, I haven't addressed the "safe-guard" comment yet because I am unclear about the meaning. I might have time later today but more probable tomorrow. I also rebased the PR, I hope that's appropriate. I put the new changes in an extra commit so you more easily review just the changes. |
Looks good. Can you please resolve conflicts? |
Implemented as AppSetting::Ignore errors as suggested by @CreepySkeleton in clap-rs#1880 (comment). This is not a complete implementation but it works already in surprisingly many situations. clap-rs#1880
dc35409
to
171dcbe
Compare
Sure :) Out of confusion I rebased on the master of my fork before. sigh |
Implemented as AppSetting::Ignore errors as suggested by
@CreepySkeleton in
#1880 (comment).
This is not a complete implementation but it works already in
surprisingly many situations.
#1880