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

convert Result<(), OsString> -> Result<(), String> #981

Closed
wants to merge 98 commits into from

Conversation

little-dude
Copy link
Contributor

@little-dude little-dude commented Jun 8, 2017

fix #848

I'm quite surprised this was as easy as changing methods signatures and fixing a few examples. Am I missing something here?


This change is Reviewable

@coveralls
Copy link

coveralls commented Jun 8, 2017

Coverage Status

Coverage remained the same at 91.927% when pulling 69c8477 on little-dude:string into 49f9dc1 on kbknapp:master.

@kbknapp
Copy link
Member

kbknapp commented Jun 10, 2017

This is a breaking change that requires bumping to 3.x.

I'm actively working on making some larger breaking changes that will include this change as well. So I'm going to hold off on this PR for a bit. Unfortunately my paid job has been crazy the past few months and I haven't had much time to work on this.

@durka
Copy link
Contributor

durka commented Oct 27, 2017

Perhaps we should roll #1088 into this.

@kbknapp kbknapp added the W: 3.x label Jan 22, 2018
…p::write_help

Contains a *MINOR* breaking change. App::write_help now requires `&mut self` instead of `&self`.

This fixes a major bug where the help message is entirely incorrect.
More can be found at clap-rs#808

I've decided to make this change because it was preventing further progress.

Anyone's code who breaks the fix is trivial:

```rust
// OLD BROKEN
let app = App::new("broken");
let mut out = io::stdout();
app.write_help(&mut out).expect("failed to write to stdout");

// NEW FIX
let mut app = App::new("broken");  // <-- let mut
let mut out = io::stdout();
app.write_help(&mut out).expect("failed to write to stdout");
```

Closes clap-rs#808
This commit primarily changes to a lazy handling of POSIX overrides by
relying on github.com/bluss/ordermap instead of the old HashMap impl.
The ordermap allows us to keep track of which arguments arrived first,
and therefore determine which ones should be removed when an override
conflict is found.

This has the added benefit of we no longer have to do the bookkeeping to
keep track and override args as they come in, we can do it once at the
end.

Finally, ordermap allows fast Vec like iteration of the keys, which we
end up doing several times. Benching is still TBD once the v3 prep is
done, but this change should have a meaningful impact.
A full list of deprecations is:

* App::version_message -> App::mut_arg
* App::version_short -> App::mut_arg
* App::help_message -> App::mut_arg
* App::help_short -> App::mut_arg
* App::from_yaml -> serde
* App::usage -> App::override_usage (+no longer assumes leading \t)
* App::help -> App::override_help
* App::template -> App::help_template
* App::args_from_usage -> App::args(&str)
* App::arg_from_usage -> App::arg(Arg::from)
* App::write_help -> &self -> &mut self (clap-rs#808)
* App::gen_completions -> clap_completions::generate
* App::gen_completions_to -> clap_completions::generate_to
* App::get_matches_safe -> App::try_get_matches (clap-rs#950 Lib Blitz naming
consistency)
* App::get_matches_from_safe -> App::try_get_matches_from (clap-rs#950 Lib Blitz
naming consistency)
* App::get_matches_safe_borrow -> App::try_get_matches_from_mut (clap-rs#950 Lib
Blitz naming consistency)
A full list of deprecations is:

* Arg::last -> ArgSettings::Last
* Arg::required -> ArgSettings::Required
* Arg::require_equals -> ArgSettings::RequireEquals
* Arg::allow_hyphen_values -> ArgSettings::AllowHyphenValues
* Arg::takes_value -> ArgSettings::TakesValue
* Arg::hide_possible_values -> ArgSettings::HidePossibleValues
* Arg::hide_default_value -> ArgSettings::HideDefaultValue
* Arg::multiple -> ArgSettings::Multiple (see Arg::multiple split)
* Arg::multiple -> ArgSettings::MultipleValues (see Arg::multiple split)
* Arg::multiple -> ArgSettings::MultipleOccurrences (see Arg::multiple split)
* Arg::global -> ArgSettings::Global
* Arg::empty_values -> ArgSettings::AllowEmptyValues
* Arg::hidden -> ArgSettings::Hidden
* Arg::case_insensitive -> ArgSettings::IgnoreCase
* Arg::use_delimiter -> ArgSettings::UseDelimiter
* Arg::require_delimiter -> ArgSettings::RequireDelimiter
* Arg::hide_env_values -> ArgSettings::HideEnvValues
* Arg::next_line_help -> ArgSettings::NextLineHelp
* Arg::set -> Arg::unset_setting (consistent naming with App)
* Arg::unset -> Arg::setting (consistent naming with App)

Relates to clap-rs#1037
kbknapp and others added 6 commits April 3, 2018 20:00
Add logic to filter based on hidden long/short.

There is still an issue with the logic in parser.rs use_long_help. This
causes invalid evaluation of whether to show/hide based on long or short help
Complete check for use_long_help, add tests
Fixes the implementation of the hiding of arguments form the short or long help on v3
Args can now be added to custom help sections. This breaks up the builder pattern a little by adding help section declarations inline, but it's the most intuitive method and doesn't require strange nesting that feels awkward.

```rust
app::new("foo")
    .arg(Arg::with_name("arg1")) // under normal headers
    .help_heading("SPECIAL")
    .arg(Arg::with_name("arg2")) // under SPECIAL: heading
```

Closes clap-rs#805
@kbknapp
Copy link
Member

kbknapp commented Apr 4, 2018

I'd like to pick this back up and change the PR to be against the v3 branch. @little-dude if you'd like to swap it over I'll merge, otherwise I can put in a new PR.

@little-dude
Copy link
Contributor Author

hey :) I can pick it back up if you don't mind waiting for this week-end. If you're in a hurry, feel free to make the PR!

@kbknapp
Copy link
Member

kbknapp commented Apr 4, 2018

No rush, v3 still has a lot of work to finish up! 😄

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.

validator_os returns Result<(), OsString>