Implement custom string parser from either `&str` or `&OsStr`. by kennytm · Pull Request #28 · TeXitoi/structopt

kennytm · 2017-11-08T09:44:09Z

Fix #2.
Fix #3.

Syntax:

#[derive(StructOpt)]
struct HexReader {
    #[structopt(short = "n", parse(try_from_str = "parse_hex"))]
    number: u32,
    #[structopt(short = "o", parse(from_os_str))]
    output: PathBuf,
}

When using structopt(parse(...)), bool and u64 will be treated as the same as Ty::Other, but Vec<T> and Option<T> remains special.
The error in try_from_str will be converted to string using .to_string() instead of .description(), as the latter is not being recommended (see Using Error well: .description() vs Display rust-lang/rust#44842 and Provide better guidance for Error::description rust-lang/rust#45249).

Fix TeXitoi#2. Fix TeXitoi#3.

TeXitoi

Great work! Thanks a lot for this great feature!

TeXitoi · 2017-11-08T10:02:29Z

+//! | `from_str`        | `fn(&str) -> T`                       | `::std::convert::From::from`    |
+//! | `try_from_str`    | `fn(&str) -> Result<T, E>`            | `::std::str::FromStr::from_str` |
+//! | `from_os_str`     | `fn(&OsStr) -> T`                     | `::std::convert::From::from`    |
+//! | `try_from_os_str` | `fn(&OsStr) -> Result<T, OsString>`   | (no default function)           |


Result<T, E> no?

Maybe say somewhere what we do with the error (call .to_string() on it).

The tricky thing is that validator_os accepts

fn validator_os<F>(self, f: F) -> Self where F: Fn(&OsStr) -> Result<(), OsString> + 'static,

Maybe it's fine to ignore the lossy conversion from OsStr to String when used for display?

OK then, better to not lose things.

TeXitoi · 2017-11-08T10:03:14Z

 //! type, and the `Arg::validator` method is set to a method using
 //! `FromStr::Err::description()` (`FromStr::Err` must implement
-//! `std::Error::Error`).
+//! `std::Error::Error`). If you would like to use a custom string parser other


We should change that to say that E::to_string will be called instead, no?

The existing implementation still uses description(). Or would you like to change that to to_string() too? 😉

Yes better to use the same thing everywhere.

TeXitoi · 2017-11-08T10:07:45Z

+        .next()
+}
+
+fn despecialize_bool_and_u64(cur_type: Ty) -> Ty {


Too specific name, prefer the intent (convert_with_custom_parse or something like that).

TeXitoi · 2017-11-08T10:18:17Z

-                validator(|s| s.parse::<#convert_type>()
-                          .map(|_| ())
-                          .map_err(|e| e.description().into()))
+            let (cur_type, validator) = match get_parser(field) {


let (cur_type, validator) = match get_parser(field).unwrap_or((Parser::TryFromStr, "::std::std::FromStr::from_str")) {

But it way not pass type check, something should be done for that (maybe just using .map(|_: #convert_type| ())).

TeXitoi · 2017-11-08T17:48:30Z

+            "test", "-a=?", "-b=?", "-c=?", "-d=?",
+        ]))
+    );
+}


Lack tests with default value:

parse(from_str) (with a Vec<u8> for example)

parse(try_from_str) (with a u64 for example)

parse(from_os_str) (PathBuf)

and an empty line at the end of file

Vec<u8> as a field doesn't work, because Vec and Option are still treated specially. I've defined a type alias instead.

BTW the following also work, but I think this is a defect:

#[derive(StructOpt, PartialEq, Debug)] struct DefaultedOpt { #[structopt(short = "b", parse(from_str))] bytes: (Vec<u8>), // <-- with '(' ... ')'. }

TeXitoi · 2017-11-08T17:58:51Z

+//! | Kind              | Signature                             | Default                         |
+//! |-------------------|---------------------------------------|---------------------------------|
+//! | `from_str`        | `fn(&str) -> T`                       | `::std::convert::From::from`    |
+//! | `try_from_str`    | `fn(&str) -> Result<T, E>`            | `::std::str::FromStr::from_str` |


That's a bit confusing that ::std::str::FromStr::from_str is the default for try_from_str and not from_str but I can't find a better idea.

TeXitoi · 2017-11-09T09:23:13Z

Thanks

TeXitoi · 2017-11-09T09:28:04Z

v0.1.4 published

* added limits * review update * change default for maniac_nodes to 4096

Implement custom string parser from either &str or &OsStr.

2749125

Fix TeXitoi#2. Fix TeXitoi#3.

TeXitoi requested changes Nov 8, 2017

View reviewed changes

Addressed review comments.

6962ec8

TeXitoi merged commit 13fc4fb into TeXitoi:master Nov 9, 2017

kennytm deleted the from-str branch November 9, 2017 09:53

CAD97 mentioned this pull request Feb 17, 2018

Re-license to Apache-2.0/MIT #71

Merged

11 tasks

Eijebong pushed a commit to Eijebong/structopt that referenced this pull request Jan 2, 2019

Decoding limits (TeXitoi#28)

9128c10

* added limits * review update * change default for maniac_nodes to 4096

Conversation

kennytm commented Nov 8, 2017

Uh oh!

TeXitoi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TeXitoi commented Nov 9, 2017

Uh oh!

TeXitoi commented Nov 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants