Skip to content
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

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

Merged
merged 2 commits into from Feb 3, 2018

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.

//!
//! | 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` |
//! | `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

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

@@ -370,6 +373,8 @@ enum Parser {
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
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.

None yet

2 participants