Skip to content

Comments

Add 'parse(from_occurrences)' parser. Don't special case 'u64'.#48

Merged
TeXitoi merged 2 commits intoTeXitoi:masterfrom
SergioBenitez:master
Feb 3, 2018
Merged

Add 'parse(from_occurrences)' parser. Don't special case 'u64'.#48
TeXitoi merged 2 commits intoTeXitoi:masterfrom
SergioBenitez:master

Conversation

@SergioBenitez
Copy link
Contributor

Fixes #30. Can change to from_occurrences if you'd like.

@TeXitoi
Copy link
Owner

TeXitoi commented Jan 29, 2018

Thanks for the contribution!

Note that's a breaking change. I would like to pass another breaking change for v0.2 (#49), so publishing v0.2 will not happen before these 2 breaking change are done.

//! | `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) |
//! | `multiple` | (no signature) | (no default function) |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

| from_occurrences | fn(u64) -> T | value as T |

It would be great to have a consistent name as from_occurrences and have the possibility to give a conversion function (for transforming the number to an Enum for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Will make this change (and the others) soon.

Copy link
Contributor Author

@SergioBenitez SergioBenitez Jan 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably take an fn(u64) -> Result<T, E> so that you can accept some number of flags and not other, i.e, if you only have three verbosity levels and want to enforce that. Nevermind. Unfortunately this isn't the way validator works for occurrences in clap.

Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor cosmetic changes

FromOsStr,
/// Parse an option to using a `fn(&OsStr) -> Result<T, OsString>` function.
TryFromOsStr,
/// Doesn't take a value. Instead, count the number of repitions.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

tests/flags.rs Outdated
assert_eq!(Opt { alice: 0, bob: 0 },
Opt::from_clap(Opt::clap().get_matches_from(&["test"])));
assert_eq!(Opt { alice: 1 },
assert_eq!(Opt { alice: 1, bob: 0},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lack space before }

@TeXitoi
Copy link
Owner

TeXitoi commented Jan 29, 2018

Great work!

@SergioBenitez
Copy link
Contributor Author

Okay! Now using parse(from_occurrences) which allows a custom parser from u64 -> T.

Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Seems perfect.

@SergioBenitez SergioBenitez changed the title Add 'parse(multiple)' parser. Don't special case 'u64'. Add 'parse(from_occurrences)' parser. Don't special case 'u64'. Jan 31, 2018
@TeXitoi TeXitoi merged commit 35423c9 into TeXitoi:master Feb 3, 2018
TeXitoi added a commit that referenced this pull request Feb 3, 2018
@CAD97 CAD97 mentioned this pull request Feb 17, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants