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

imp: make Arg::validator more flexible (v3) #1243

Merged
merged 2 commits into from
Apr 5, 2018

Conversation

durka
Copy link
Contributor

@durka durka commented Apr 5, 2018

From #1088.

Makes the validator functions more flexible by changing the return type from Result<(), String> to Result<O, E> where O is anything and E is anything convertible to a String.

This allows, for example, using the same function for validating and parsing your argument.


This change is Reviewable

durka and others added 2 commits April 4, 2018 20:34
Makes the validator functions more flexible by changing the return
type from Result<(), String> to Result<O, E> where O is anything
and E is anything convertible to a String.

This allows, for example, using the same function for validating
and parsing your argument.

Breaking change (albeit tiny) due to function signature change.
@mention-bot
Copy link

@durka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbknapp to be a potential reviewer.

@durka
Copy link
Contributor Author

durka commented Apr 5, 2018

I don't think it requires additional turbofishes but I didn't test very extensively.

@kbknapp
Copy link
Member

kbknapp commented Apr 5, 2018

Here's the test failure so you don't have to dig through the logs:

Running `/home/travis/build/kbknapp/clap-rs/target/debug/deps/env-a56ad90d3ded717c`
running 15 tests
test env_os ... ok
test env ... ok
test multiple_no_delimiter ... ok
test multiple_one ... ok
test no_env ... ok
test multiple_three ... ok
test not_possible_value ... ok
test opt_user_override ... ok
test positionals_user_override ... ok
test positionals ... ok
test possible_value ... ok
test validator ... ok
test validator_invalid ... ok
test validator_output ... FAILED
test with_default ... ok
failures:
---- validator_output stdout ----
	thread 'validator_output' panicked at 'called `Result::unwrap()` on an `Err` value: Error { message: "error: Invalid value for \'<arg>\': invalid digit found in string", kind: ValueValidation, info: None }', libcore/result.rs:945:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    validator_output
test result: FAILED. 14 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--test env'

@durka
Copy link
Contributor Author

durka commented Apr 5, 2018

WTF, that test passes locally. I just copied it from the validator test.

     Running target/debug/deps/env-02b35348c36b6f4b

running 15 tests
test multiple_one ... ok
test env ... ok
test env_os ... ok
test multiple_no_delimiter ... ok
test multiple_three ... ok
test no_env ... ok
test not_possible_value ... ok
test positionals ... ok
test opt_user_override ... ok
test positionals_user_override ... ok
test possible_value ... ok
test validator ... ok
test validator_invalid ... ok
test validator_output ... ok
test with_default ... ok

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

@kbknapp
Copy link
Member

kbknapp commented Apr 5, 2018

Yeah super strange, cause it passes locally for me too! I'll restart the travis build.

@kbknapp
Copy link
Member

kbknapp commented Apr 5, 2018

kevin@beefcake: ~/Projects/clap-rs 
➜ cargo test --test env -- validator_output
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running target/debug/deps/env-c0e4ef4fb869aef2

running 1 test
test validator_output ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 14 filtered out

@durka
Copy link
Contributor Author

durka commented Apr 5, 2018

Looks good now.

@kbknapp
Copy link
Member

kbknapp commented Apr 5, 2018

Thanks 👍

@kbknapp kbknapp merged commit 9981bac into clap-rs:v3-master Apr 5, 2018
@kbknapp kbknapp mentioned this pull request Apr 5, 2018
87 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants