feat(Validators): adds ability to validate invalid UTF-8 #784
Conversation
|
Strange, I wonder why this is showing such a large diff? I'll take a look at this soon and should be able to merge for 2.20 👍 |
|
Aaah ok, I see why. In your fork, it looks like you've moved the files from Thanks! |
|
OK thanks, yh I'll make the changes and resubmit :) |
glowing-chemist
changed the title
| @@ -1525,6 +1525,7 @@ impl<'n, 'e> AnyArg<'n, 'e> for App<'n, 'e> { | |||
| fn num_vals(&self) -> Option<u64> { None } | |||
| fn possible_vals(&self) -> Option<&[&'e str]> { None } | |||
| fn validator(&self) -> Option<&Rc<Fn(String) -> StdResult<(), String>>> { None } | |||
| fn validator_os(&self) -> Option<&Rc<Fn(String) -> StdResult<(), String>>> { None } | |||
There was a problem hiding this comment.
This will need to be an OsString instead of a String to allow for invalid UTF-8
| @@ -1533,6 +1533,11 @@ impl<'a, 'b> Parser<'a, 'b> | |||
| return Err(Error::value_validation(Some(arg), e, self.color())); | |||
| } | |||
| } | |||
| if let Some(vtor) = arg.validator_os() { | |||
| if let Err(e) = vtor(val.to_string_lossy().into_owned()) { | |||
There was a problem hiding this comment.
Once the String->OsString is done, this should be able to change to just vtor(val)
|
Whichever you're able to get working |
|
I've changed all relevant Strings' to OsStrings and OsStr respectfully :) |
| if let Err(e) = vtor(val.to_string_lossy().into_owned()) { | ||
| return Err(Error::value_validation(Some(arg), e, self.color())); | ||
| if let Err(e) = vtor(val) { | ||
| return Err(Error::value_validation(Some(arg), e.into_string().unwrap_or("error invalid UTF-8".to_string()), self.color())); |
There was a problem hiding this comment.
This will change the users custom error message to this generic one if the user returns an OsString with invalid UTF-8 (which is almost expected because they're using validator_os in the first place). So we need to change this to something like &e.to_string_lossy()
There was a problem hiding this comment.
Oh yh had a bit of a brain fart there :p will fix that now
|
Look great! Once the tests pass, I'll merge 👍 Thanks again for tackling this! |
1 similar comment
|
@homu r+ |
|
📌 Commit a7186fa has been approved by |
feat(Validators): adds ability to validate invalid UTF-8 added validator_os
feat(Validators): adds ability to validate invalid UTF-8 added validator_os
|
☀️ Test successful - status |
added validator_os