Skip to content

Commit

Permalink
fix: fixes issues and potential regressions with global args values n…
Browse files Browse the repository at this point in the history 
…ot being propagated properly or at all

Closes #1010
Closes #1061
Closes #978
  • Loading branch information
@kbknapp
kbknapp committed Oct 24, 2017
1 parent ead076f commit a43f9dd
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 93 deletions.
88 changes: 18 additions & 70 deletions src/app/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use std::path::Path;
use std::process;
use std::rc::Rc;
use std::result::Result as StdResult;
use std::collections::HashMap;

// Third Party
#[cfg(feature = "yaml")]
Expand All @@ -25,7 +24,7 @@ use yaml_rust::Yaml;
// Internal
use app::help::Help;
use app::parser::Parser;
use args::{AnyArg, Arg, ArgGroup, ArgMatcher, ArgMatches, ArgSettings, MatchedArg, SubCommand};
use args::{AnyArg, Arg, ArgGroup, ArgMatcher, ArgMatches, ArgSettings};
use errors::Result as ClapResult;
pub use self::settings::AppSettings;
use completions::Shell;
Expand Down Expand Up @@ -610,11 +609,11 @@ impl<'a, 'b> App<'a, 'b> {
self
}

/// Enables a single setting that is propogated *down* through all child [`SubCommand`]s.
/// Enables a single setting that is propagated down through all child [`SubCommand`]s.
///
/// See [`AppSettings`] for a full list of possibilities and examples.
///
/// **NOTE**: The setting is *only* propogated *down* and not up through parent commands.
/// **NOTE**: The setting is *only* propagated *down* and not up through parent commands.
///
/// # Examples
///
Expand All @@ -632,11 +631,11 @@ impl<'a, 'b> App<'a, 'b> {
self
}

/// Enables multiple settings which are propogated *down* through all child [`SubCommand`]s.
/// Enables multiple settings which are propagated *down* through all child [`SubCommand`]s.
///
/// See [`AppSettings`] for a full list of possibilities and examples.
///
/// **NOTE**: The setting is *only* propogated *down* and not up through parent commands.
/// **NOTE**: The setting is *only* propagated *down* and not up through parent commands.
///
/// # Examples
///
Expand Down Expand Up @@ -1154,8 +1153,8 @@ impl<'a, 'b> App<'a, 'b> {
pub fn print_help(&mut self) -> ClapResult<()> {
// If there are global arguments, or settings we need to propgate them down to subcommands
// before parsing incase we run into a subcommand
self.p.propogate_globals();
self.p.propogate_settings();
self.p.propagate_globals();
self.p.propagate_settings();
self.p.derive_display_order();

self.p.create_help_and_version();
Expand Down Expand Up @@ -1184,8 +1183,8 @@ impl<'a, 'b> App<'a, 'b> {
pub fn print_long_help(&mut self) -> ClapResult<()> {
// If there are global arguments, or settings we need to propgate them down to subcommands
// before parsing incase we run into a subcommand
self.p.propogate_globals();
self.p.propogate_settings();
self.p.propagate_globals();
self.p.propagate_settings();
self.p.derive_display_order();

self.p.create_help_and_version();
Expand All @@ -1200,7 +1199,7 @@ impl<'a, 'b> App<'a, 'b> {
/// **NOTE:** clap has the ability to distinguish between "short" and "long" help messages
/// depending on if the user ran [`-h` (short)] or [`--help` (long)]
///
/// **NOTE:** There is a known bug where this method does not write propogated global arguments
/// **NOTE:** There is a known bug where this method does not write propagated global arguments
/// or autogenerated arguments (i.e. the default help/version args). Prefer
/// [`App::write_long_help`] instead if possibe!
///
Expand All @@ -1221,8 +1220,8 @@ impl<'a, 'b> App<'a, 'b> {
// https://github.com/kbknapp/clap-rs/issues/808
// If there are global arguments, or settings we need to propgate them down to subcommands
// before parsing incase we run into a subcommand
// self.p.propogate_globals();
// self.p.propogate_settings();
// self.p.propagate_globals();
// self.p.propagate_settings();
// self.p.derive_display_order();
// self.p.create_help_and_version();

Expand All @@ -1248,8 +1247,8 @@ impl<'a, 'b> App<'a, 'b> {
/// [`-h` (short)]: ./struct.Arg.html#method.help
/// [`--help` (long)]: ./struct.Arg.html#method.long_help
pub fn write_long_help<W: Write>(&mut self, w: &mut W) -> ClapResult<()> {
self.p.propogate_globals();
self.p.propogate_settings();
self.p.propagate_globals();
self.p.propagate_settings();
self.p.derive_display_order();
self.p.create_help_and_version();

Expand Down Expand Up @@ -1587,8 +1586,8 @@ impl<'a, 'b> App<'a, 'b> {
{
// If there are global arguments, or settings we need to propgate them down to subcommands
// before parsing incase we run into a subcommand
self.p.propogate_globals();
self.p.propogate_settings();
self.p.propagate_globals();
self.p.propagate_settings();
self.p.derive_display_order();

let mut matcher = ArgMatcher::new();
Expand Down Expand Up @@ -1620,62 +1619,11 @@ impl<'a, 'b> App<'a, 'b> {
return Err(e);
}

if self.p.is_set(AppSettings::PropagateGlobalValuesDown) {
let global_arg_vec : Vec<&str> = (&self).p.global_args.iter().map(|ga| ga.b.name).collect();
let mut global_arg_to_value_map = HashMap::new();
matcher.get_global_values(&global_arg_vec, &mut global_arg_to_value_map);
if let Some(ref mut sc) = matcher.0.subcommand {
self.handle_subcommand_globals(sc, &mut global_arg_to_value_map, &global_arg_vec);
}
}
let global_arg_vec : Vec<&str> = (&self).p.global_args.iter().map(|ga| ga.b.name).collect();
matcher.propagate_globals(&global_arg_vec);

Ok(matcher.into())
}

fn handle_subcommand_globals(&self, subcommand : &mut Box<SubCommand<'a>>, arg_value_map: &mut HashMap<&'a str, Vec<OsString>>, global_arg_vec: &Vec<&'a str>) {
let empty_vec_reference = &vec![];
for global_arg in global_arg_vec.iter() {
let sma = (*subcommand).matches.args.entry(global_arg).or_insert_with(|| {
let vals = arg_value_map.get(global_arg).unwrap_or(empty_vec_reference);
let mut gma = MatchedArg::new();
gma.occurs += 1;
if !vals.is_empty() {
gma.vals = vals.clone();
}
gma
});
if sma.vals.is_empty() {
let vals = arg_value_map.get(global_arg).unwrap_or(empty_vec_reference);
sma.vals = vals.clone();
} else {
arg_value_map.insert(global_arg, sma.vals.clone());
}
}
if let Some(ref mut inner_sub) = subcommand.matches.subcommand {
self.handle_subcommand_globals(inner_sub, arg_value_map, global_arg_vec);
}
self.fill_in_missing_globals(subcommand, arg_value_map, global_arg_vec);
}

fn fill_in_missing_globals(&self, subcommand : &mut Box<SubCommand<'a>>, arg_value_map: &mut HashMap<&'a str, Vec<OsString>>, global_arg_vec: &Vec<&'a str>) {
let empty_vec_reference = &vec![];
for global_arg in global_arg_vec.iter() {
let sma = (*subcommand).matches.args.entry(global_arg).or_insert_with(|| {
let vals = arg_value_map.get(global_arg).unwrap_or(empty_vec_reference);
let mut gma = MatchedArg::new();
gma.occurs += 1;
if !vals.is_empty() {
gma.vals = vals.clone();
}
gma
});
if sma.vals.is_empty() {
let vals = arg_value_map.get(global_arg).unwrap_or(empty_vec_reference);
sma.vals = vals.clone();
}
}
}

}

#[cfg(feature = "yaml")]
Expand Down
28 changes: 14 additions & 14 deletions src/app/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,12 @@ impl<'a, 'b> Parser<'a, 'b>
}

pub fn gen_completions_to<W: Write>(&mut self, for_shell: Shell, buf: &mut W) {
if !self.is_set(AS::Propogated) {
self.propogate_help_version();
if !self.is_set(AS::Propagated) {
self.propagate_help_version();
self.build_bin_names();
self.propogate_globals();
self.propogate_settings();
self.set(AS::Propogated);
self.propagate_globals();
self.propagate_settings();
self.set(AS::Propagated);
}

ComplGen::new(self).generate(for_shell, buf)
Expand Down Expand Up @@ -366,12 +366,12 @@ impl<'a, 'b> Parser<'a, 'b>
self.subcommands.push(subcmd);
}

pub fn propogate_settings(&mut self) {
debugln!("Parser::propogate_settings: self={}, g_settings={:#?}",
pub fn propagate_settings(&mut self) {
debugln!("Parser::propagate_settings: self={}, g_settings={:#?}",
self.meta.name,
self.g_settings);
for sc in &mut self.subcommands {
debugln!("Parser::propogate_settings: sc={}, settings={:#?}, g_settings={:#?}",
debugln!("Parser::propagate_settings: sc={}, settings={:#?}, g_settings={:#?}",
sc.p.meta.name,
sc.p.settings,
sc.p.g_settings);
Expand All @@ -393,7 +393,7 @@ impl<'a, 'b> Parser<'a, 'b>
sc.p.meta.term_w = self.meta.term_w;
sc.p.meta.max_w = self.meta.max_w;
}
sc.p.propogate_settings();
sc.p.propagate_settings();
}
}

Expand Down Expand Up @@ -607,7 +607,7 @@ impl<'a, 'b> Parser<'a, 'b>
true
}

pub fn propogate_globals(&mut self) {
pub fn propagate_globals(&mut self) {
for sc in &mut self.subcommands {
// We have to create a new scope in order to tell rustc the borrow of `sc` is
// done and to recursively call this method
Expand All @@ -616,7 +616,7 @@ impl<'a, 'b> Parser<'a, 'b>
sc.p.add_arg_ref(a);
}
}
sc.p.propogate_globals();
sc.p.propagate_globals();
}
}

Expand Down Expand Up @@ -1075,11 +1075,11 @@ impl<'a, 'b> Parser<'a, 'b>
}


fn propogate_help_version(&mut self) {
debugln!("Parser::propogate_help_version;");
fn propagate_help_version(&mut self) {
debugln!("Parser::propagate_help_version;");
self.create_help_and_version();
for sc in &mut self.subcommands {
sc.p.propogate_help_version();
sc.p.propagate_help_version();
}
}

Expand Down
48 changes: 39 additions & 9 deletions src/args/arg_matcher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,45 @@ impl<'a> Default for ArgMatcher<'a> {
impl<'a> ArgMatcher<'a> {
pub fn new() -> Self { ArgMatcher::default() }

pub fn get_global_values(&mut self, global_arg_vec : &Vec<&'a str>, vals_map: &mut HashMap<&'a str, Vec<OsString>>) {
for global_arg in global_arg_vec.iter() {
let vals: Vec<_> = if let Some(ma) = self.get(global_arg) {
ma.vals.clone()
} else {
debugln!("ArgMatcher::propagate: arg wasn't used");
Vec::new()
};
vals_map.insert(global_arg, vals);
pub fn propagate_globals(&mut self, global_arg_vec : &[&'a str]) {
debugln!("ArgMatcher::get_global_values: global_arg_vec={:?}", global_arg_vec);
let mut vals_map = HashMap::new();
self.fill_in_global_values(global_arg_vec, &mut vals_map);
}

fn fill_in_global_values(
&mut self,
global_arg_vec: &[&'a str],
vals_map: &mut HashMap<&'a str, MatchedArg>
) {
for global_arg in global_arg_vec {
if let Some(ma) = self.get(global_arg) {
// We have to check if the parent's global arg wasn't used but still exists
// such as from a default value.
//
// For example, `myprog subcommand --global-arg=value` where --global-arg defines
// a default value of `other` myprog would have an existing MatchedArg for
// --global-arg where the value is `other`, however the occurs will be 0.
let to_update = if let Some(parent_ma) = vals_map.get(global_arg) {
if parent_ma.occurs > 0 && ma.occurs == 0 {
parent_ma.clone()
} else {
ma.clone()
}
} else {
ma.clone()
};
vals_map.insert(global_arg, to_update);
}
}
if let Some(ref mut sc) = self.0.subcommand {
let mut am = ArgMatcher(mem::replace(&mut sc.matches, ArgMatches::new()));
am.fill_in_global_values(global_arg_vec, vals_map);
mem::swap(&mut am.0, &mut sc.matches);
}

for (name, matched_arg) in vals_map.into_iter() {
self.0.args.insert(name, matched_arg.clone());
}
}

Expand Down

0 comments on commit a43f9dd

Please sign in to comment.