Auto merge of #709 - Nemo157:derive_order_pr, r=kbknapp

homu · homu · commit 0f8764cbe8a7 · 2016-10-27T13:17:41.000+09:00
Derive display order after propagation

Don't attempt to change the display order of flags/options until any app settings have been propagated down from a parent `App` in case `DeriveDisplayOrder` and/or `UnifiedHelpMessage` are propagated.

Adds tests that try to stress the combinations of `DeriveDisplayOrder` and `UnifiedHelpMessage` along with propagating them to subcommands and explicitly setting a display order.
diff --git a/src/app/mod.rs b/src/app/mod.rs
@@ -1318,6 +1318,7 @@ impl<'a, 'b> App<'a, 'b> {
         // before parsing incase we run into a subcommand
         self.p.propogate_globals();
         self.p.propogate_settings();
+        self.p.derive_display_order();
 
         let mut matcher = ArgMatcher::new();
 
diff --git a/src/app/parser.rs b/src/app/parser.rs
@@ -179,23 +179,11 @@ impl<'a, 'b> Parser<'a, 'b>
             self.positionals.insert(i, pb);
         } else if a.is_set(ArgSettings::TakesValue) {
             let mut ob = OptBuilder::from_arg(a, &mut self.required);
-            if self.settings.is_set(AppSettings::DeriveDisplayOrder) && a.disp_ord == 999 {
-                ob.disp_ord = if self.settings.is_set(AppSettings::UnifiedHelpMessage) {
-                    self.flags.len() + self.opts.len()
-                } else {
-                    self.opts.len()
-                };
-            }
+            ob.unified_ord = self.flags.len() + self.opts.len();
             self.opts.push(ob);
         } else {
             let mut fb = FlagBuilder::from(a);
-            if self.settings.is_set(AppSettings::DeriveDisplayOrder) && a.disp_ord == 999 {
-                fb.disp_ord = if self.settings.is_set(AppSettings::UnifiedHelpMessage) {
-                    self.flags.len() + self.opts.len()
-                } else {
-                    self.flags.len()
-                };
-            }
+            fb.unified_ord = self.flags.len() + self.opts.len();
             self.flags.push(fb);
         }
         if a.is_set(ArgSettings::Global) {
@@ -242,9 +230,6 @@ impl<'a, 'b> Parser<'a, 'b>
             sdebugln!("No");
         }
 
-        if self.settings.is_set(AppSettings::DeriveDisplayOrder) {
-            subcmd.p.meta.disp_ord = self.subcommands.len();
-        }
         self.subcommands.push(subcmd);
     }
 
@@ -275,6 +260,24 @@ impl<'a, 'b> Parser<'a, 'b>
         }
     }
 
+    pub fn derive_display_order(&mut self) {
+        if self.settings.is_set(AppSettings::DeriveDisplayOrder) {
+            let unified = self.settings.is_set(AppSettings::UnifiedHelpMessage);
+            for (i, o) in self.opts.iter_mut().enumerate().filter(|&(_, ref o)| o.disp_ord == 999) {
+                o.disp_ord = if unified { o.unified_ord } else { i };
+            }
+            for (i, f) in self.flags.iter_mut().enumerate().filter(|&(_, ref f)| f.disp_ord == 999) {
+                f.disp_ord = if unified { f.unified_ord } else { i };
+            }
+            for (i, sc) in &mut self.subcommands.iter_mut().enumerate().filter(|&(_, ref sc)| sc.p.meta.disp_ord == 999) {
+                sc.p.meta.disp_ord = i;
+            }
+        }
+        for sc in &mut self.subcommands {
+            sc.p.derive_display_order();
+        }
+    }
+
     pub fn required(&self) -> Iter<&str> {
         self.required.iter()
     }
diff --git a/src/args/arg_builder/flag.rs b/src/args/arg_builder/flag.rs
@@ -25,6 +25,7 @@ pub struct FlagBuilder<'n, 'e> {
     pub overrides: Option<Vec<&'e str>>,
     pub settings: ArgFlags,
     pub disp_ord: usize,
+    pub unified_ord: usize,
 }
 
 impl<'n, 'e> Default for FlagBuilder<'n, 'e> {
@@ -40,6 +41,7 @@ impl<'n, 'e> Default for FlagBuilder<'n, 'e> {
             overrides: None,
             settings: ArgFlags::new(),
             disp_ord: 999,
+            unified_ord: 999,
         }
     }
 }
@@ -77,6 +79,7 @@ impl<'a, 'b, 'z> From<&'z Arg<'a, 'b>> for FlagBuilder<'a, 'b> {
             requires: a.requires.clone(),
             settings: a.settings,
             disp_ord: a.disp_ord,
+            ..Default::default()
         }
     }
 }
@@ -106,6 +109,7 @@ impl<'n, 'e> Clone for FlagBuilder<'n, 'e> {
             requires: self.requires.clone(),
             settings: self.settings,
             disp_ord: self.disp_ord,
+            unified_ord: self.unified_ord,
         }
     }
 }
diff --git a/src/args/arg_builder/option.rs b/src/args/arg_builder/option.rs
@@ -31,6 +31,7 @@ pub struct OptBuilder<'n, 'e> {
     pub val_delim: Option<char>,
     pub default_val: Option<&'n str>,
     pub disp_ord: usize,
+    pub unified_ord: usize,
     pub r_unless: Option<Vec<&'e str>>,
 }
 
@@ -55,6 +56,7 @@ impl<'n, 'e> Default for OptBuilder<'n, 'e> {
             val_delim: Some(','),
             default_val: None,
             disp_ord: 999,
+            unified_ord: 999,
             r_unless: None,
         }
     }
@@ -177,6 +179,7 @@ impl<'n, 'e> Clone for OptBuilder<'n, 'e> {
             requires: self.requires.clone(),
             settings: self.settings,
             disp_ord: self.disp_ord,
+            unified_ord: self.unified_ord,
             num_vals: self.num_vals,
             min_vals: self.min_vals,
             max_vals: self.max_vals,
diff --git a/tests/derive_order.rs b/tests/derive_order.rs
@@ -0,0 +1,249 @@
+extern crate clap;
+
+use std::str;
+
+use clap::{App, Arg, SubCommand, AppSettings};
+
+fn condense_whitespace(s: &str) -> String {
+    s.split_whitespace().collect::<Vec<_>>().join(" ")
+}
+
+fn normalize(s: &str) -> Vec<String> {
+    s.lines().map(|l| l.trim()).filter(|l| !l.is_empty()).map(condense_whitespace).collect()
+}
+
+fn get_help(app: App, args: &[&'static str]) -> Vec<String> {
+    let res = app.get_matches_from_safe(args.iter().chain(["--help"].iter()));
+    normalize(&*res.unwrap_err().message)
+}
+
+#[test]
+fn no_derive_order() {
+    let app = App::new("test")
+        .args(&[
+            Arg::with_name("flag_b").long("flag_b").help("first flag"),
+            Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+            Arg::with_name("flag_a").long("flag_a").help("second flag"),
+            Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+        ]);
+
+    assert_eq!(get_help(app, &["test"]), normalize("
+        test
+
+        USAGE:
+            test [FLAGS] [OPTIONS]
+
+        FLAGS:
+                --flag_a     second flag
+                --flag_b     first flag
+            -h, --help       Prints help information
+            -V, --version    Prints version information
+
+        OPTIONS:
+                --option_a <option_a>    second option
+                --option_b <option_b>    first option
+    "));
+}
+
+#[test]
+fn derive_order() {
+    let app = App::new("test")
+        .setting(AppSettings::DeriveDisplayOrder)
+        .args(&[
+            Arg::with_name("flag_b").long("flag_b").help("first flag"),
+            Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+            Arg::with_name("flag_a").long("flag_a").help("second flag"),
+            Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+        ]);
+
+    assert_eq!(get_help(app, &["test"]), normalize("
+        test
+
+        USAGE:
+            test [FLAGS] [OPTIONS]
+
+        FLAGS:
+                --flag_b     first flag
+                --flag_a     second flag
+            -h, --help       Prints help information
+            -V, --version    Prints version information
+
+        OPTIONS:
+                --option_b <option_b>    first option
+                --option_a <option_a>    second option
+    "));
+}
+
+#[test]
+fn unified_help() {
+    let app = App::new("test")
+        .setting(AppSettings::UnifiedHelpMessage)
+        .args(&[
+            Arg::with_name("flag_b").long("flag_b").help("first flag"),
+            Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+            Arg::with_name("flag_a").long("flag_a").help("second flag"),
+            Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+        ]);
+
+    assert_eq!(get_help(app, &["test"]), normalize("
+        test
+
+        USAGE:
+            test [OPTIONS]
+
+        OPTIONS:
+                --flag_a     second flag
+                --flag_b     first flag
+            -h, --help       Prints help information
+                --option_a <option_a>    second option
+                --option_b <option_b>    first option
+            -V, --version    Prints version information
+    "));
+}
+
+#[test]
+fn unified_help_and_derive_order() {
+    let app = App::new("test")
+        .setting(AppSettings::DeriveDisplayOrder)
+        .setting(AppSettings::UnifiedHelpMessage)
+        .args(&[
+            Arg::with_name("flag_b").long("flag_b").help("first flag"),
+            Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+            Arg::with_name("flag_a").long("flag_a").help("second flag"),
+            Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+        ]);
+
+    assert_eq!(get_help(app, &["test"]), normalize("
+        test
+
+        USAGE:
+            test [OPTIONS]
+
+        OPTIONS:
+                --flag_b     first flag
+                --option_b <option_b>    first option
+                --flag_a     second flag
+                --option_a <option_a>    second option
+            -h, --help       Prints help information
+            -V, --version    Prints version information
+    "));
+}
+
+#[test]
+fn derive_order_subcommand_propagate() {
+    let app = App::new("test")
+        .global_setting(AppSettings::DeriveDisplayOrder)
+        .subcommand(SubCommand::with_name("sub")
+            .args(&[
+                Arg::with_name("flag_b").long("flag_b").help("first flag"),
+                Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+                Arg::with_name("flag_a").long("flag_a").help("second flag"),
+                Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+            ]));
+
+    assert_eq!(get_help(app, &["test", "sub"]), normalize("
+        test-sub
+
+        USAGE:
+            test sub [FLAGS] [OPTIONS]
+
+        FLAGS:
+                --flag_b     first flag
+                --flag_a     second flag
+            -h, --help       Prints help information
+            -V, --version    Prints version information
+
+        OPTIONS:
+                --option_b <option_b>    first option
+                --option_a <option_a>    second option
+    "));
+}
+
+#[test]
+fn unified_help_subcommand_propagate() {
+    let app = App::new("test")
+        .global_setting(AppSettings::UnifiedHelpMessage)
+        .subcommand(SubCommand::with_name("sub")
+            .args(&[
+                Arg::with_name("flag_b").long("flag_b").help("first flag"),
+                Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+                Arg::with_name("flag_a").long("flag_a").help("second flag"),
+                Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+            ]));
+
+    assert_eq!(get_help(app, &["test", "sub"]), normalize("
+        test-sub
+
+        USAGE:
+            test sub [OPTIONS]
+
+        OPTIONS:
+                --flag_a     second flag
+                --flag_b     first flag
+            -h, --help       Prints help information
+                --option_a <option_a>    second option
+                --option_b <option_b>    first option
+            -V, --version    Prints version information
+
+    "));
+}
+
+#[test]
+fn unified_help_and_derive_order_subcommand_propagate() {
+    let app = App::new("test")
+        .global_setting(AppSettings::DeriveDisplayOrder)
+        .global_setting(AppSettings::UnifiedHelpMessage)
+        .subcommand(SubCommand::with_name("sub")
+            .args(&[
+                Arg::with_name("flag_b").long("flag_b").help("first flag"),
+                Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+                Arg::with_name("flag_a").long("flag_a").help("second flag"),
+                Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+            ]));
+
+    assert_eq!(get_help(app, &["test", "sub"]), normalize("
+        test-sub
+
+        USAGE:
+            test sub [OPTIONS]
+
+        OPTIONS:
+                --flag_b     first flag
+                --option_b <option_b>    first option
+                --flag_a     second flag
+                --option_a <option_a>    second option
+            -h, --help       Prints help information
+            -V, --version    Prints version information
+
+    "));
+}
+
+#[test]
+fn unified_help_and_derive_order_subcommand_propagate_with_explicit_display_order() {
+    let app = App::new("test")
+        .global_setting(AppSettings::DeriveDisplayOrder)
+        .global_setting(AppSettings::UnifiedHelpMessage)
+        .subcommand(SubCommand::with_name("sub")
+            .args(&[
+                Arg::with_name("flag_b").long("flag_b").help("first flag"),
+                Arg::with_name("option_b").long("option_b").takes_value(true).help("first option"),
+                Arg::with_name("flag_a").long("flag_a").help("second flag").display_order(0),
+                Arg::with_name("option_a").long("option_a").takes_value(true).help("second option"),
+            ]));
+
+    assert_eq!(get_help(app, &["test", "sub"]), normalize("
+        test-sub
+
+        USAGE:
+            test sub [OPTIONS]
+
+        OPTIONS:
+                --flag_a     second flag
+                --flag_b     first flag
+                --option_b <option_b>    first option
+                --option_a <option_a>    second option
+            -h, --help       Prints help information
+            -V, --version    Prints version information
+
+    "));
+}

Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@ pub struct FlagBuilder<'n, 'e> {`
`25`	`25`	`pub overrides: Option<Vec<&'e str>>,`
`26`	`26`	`pub settings: ArgFlags,`
`27`	`27`	`pub disp_ord: usize,`
	`28`	`+ pub unified_ord: usize,`
`28`	`29`	`}`
`29`	`30`
`30`	`31`	`impl<'n, 'e> Default for FlagBuilder<'n, 'e> {`
`@@ -40,6 +41,7 @@ impl<'n, 'e> Default for FlagBuilder<'n, 'e> {`
`40`	`41`	`overrides: None,`
`41`	`42`	`settings: ArgFlags::new(),`
`42`	`43`	`disp_ord: 999,`
	`44`	`+ unified_ord: 999,`
`43`	`45`	`}`
`44`	`46`	`}`
`45`	`47`	`}`
`@@ -77,6 +79,7 @@ impl<'a, 'b, 'z> From<&'z Arg<'a, 'b>> for FlagBuilder<'a, 'b> {`
`77`	`79`	`requires: a.requires.clone(),`
`78`	`80`	`settings: a.settings,`
`79`	`81`	`disp_ord: a.disp_ord,`
	`82`	`+ ..Default::default()`
`80`	`83`	`}`
`81`	`84`	`}`
`82`	`85`	`}`
`@@ -106,6 +109,7 @@ impl<'n, 'e> Clone for FlagBuilder<'n, 'e> {`
`106`	`109`	`requires: self.requires.clone(),`
`107`	`110`	`settings: self.settings,`
`108`	`111`	`disp_ord: self.disp_ord,`
	`112`	`+ unified_ord: self.unified_ord,`
`109`	`113`	`}`
`110`	`114`	`}`
`111`	`115`	`}`