From 2f9846d0487a7485ad4eec621871dca1fd0db328 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Sat, 12 Dec 2020 15:05:42 +0800 Subject: [PATCH] Lazy subcommand propagation, avoid redundant _build() function call rustfmt and clippy Indepth cleaning Apply suggestions --- clap_generate/src/generators/mod.rs | 4 +- clap_generate/src/lib.rs | 6 +- src/build/app/mod.rs | 111 +++++++++++----------------- src/build/app/tests.rs | 8 +- src/build/usage_parser.rs | 8 +- src/parse/parser.rs | 9 --- 6 files changed, 55 insertions(+), 91 deletions(-) diff --git a/clap_generate/src/generators/mod.rs b/clap_generate/src/generators/mod.rs index 7c1701b1..adeb8ab9 100644 --- a/clap_generate/src/generators/mod.rs +++ b/clap_generate/src/generators/mod.rs @@ -117,7 +117,7 @@ pub trait Generator { /// Gets all the short options, their visible aliases and flags of a [`clap::App`](../clap/struct.App.html). /// Includes `h` and `V` depending on the [`clap::AppSettings`](../clap/enum.AppSettings.html). - fn shorts_and_visible_aliases<'help>(p: &App<'help>) -> Vec { + fn shorts_and_visible_aliases(p: &App) -> Vec { debug!("shorts: name={}", p.get_name()); let mut shorts_and_visible_aliases: Vec = p @@ -155,7 +155,7 @@ pub trait Generator { /// Gets all the long options and flags of a [`clap::App`](../clap/struct.App.html). /// Includes `help` and `version` depending on the [`clap::AppSettings`](../clap/enum.AppSettings.html). - fn longs<'help>(p: &App<'help>) -> Vec { + fn longs(p: &App) -> Vec { debug!("longs: name={}", p.get_name()); let mut longs: Vec = p diff --git a/clap_generate/src/lib.rs b/clap_generate/src/lib.rs index 5eda6c2f..426c3a58 100644 --- a/clap_generate/src/lib.rs +++ b/clap_generate/src/lib.rs @@ -181,10 +181,8 @@ where { app.set_bin_name(bin_name); - if !app.is_set(clap::AppSettings::Built) { - app._build(); - app._build_bin_names(); - } + app._build(); + app._build_bin_names(); G::generate(app, buf) } diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 86c410b6..0d819562 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -31,17 +31,6 @@ use crate::{ Result as ClapResult, INTERNAL_ERROR_MSG, }; -// @TODO FIXME (@CreepySkeleton): some of these variants (None) are never constructed -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) enum Propagation { - To(Id), - Full, - #[cfg_attr(not(test), allow(unused))] - NextLevel, - #[allow(unused)] - None, -} - /// Represents a command line interface which is made up of all possible /// command line arguments and subcommands. Interface arguments and settings are /// configured using the "builder pattern." Once all configuration is complete, @@ -1958,9 +1947,7 @@ impl<'help> App<'help> { pub fn generate_usage(&mut self) -> String { // If there are global arguments, or settings we need to propagate them down to subcommands // before parsing incase we run into a subcommand - if !self.settings.is_set(AppSettings::Built) { - self._build(); - } + self._build(); let mut parser = Parser::new(self); parser._build(); @@ -2267,9 +2254,7 @@ impl<'help> App<'help> { // If there are global arguments, or settings we need to propagate them down to subcommands // before parsing in case we run into a subcommand - if !self.settings.is_set(AppSettings::Built) { - self._build(); - } + self._build(); // do the real parsing let mut parser = Parser::new(self); @@ -2292,47 +2277,50 @@ impl<'help> App<'help> { #[doc(hidden)] pub fn _build(&mut self) { debug!("App::_build"); + if !self.settings.is_set(AppSettings::Built) { + // Make sure all the globally set flags apply to us as well + self.settings = self.settings | self.g_settings; - // Make sure all the globally set flags apply to us as well - self.settings = self.settings | self.g_settings; + self._propagate(); - self._propagate(Propagation::Full); + self._derive_display_order(); + self._create_help_and_version(); - self._derive_display_order(); - self._create_help_and_version(); + let mut pos_counter = 1; + for a in self.args.args.iter_mut() { + // Fill in the groups + for g in &a.groups { + if let Some(ag) = self.groups.iter_mut().find(|grp| grp.id == *g) { + ag.args.push(a.id.clone()); + } else { + let mut ag = ArgGroup::with_id(g.clone()); + ag.args.push(a.id.clone()); + self.groups.push(ag); + } + } - let mut pos_counter = 1; - for a in self.args.args.iter_mut() { - // Fill in the groups - for g in &a.groups { - if let Some(ag) = self.groups.iter_mut().find(|grp| grp.id == *g) { - ag.args.push(a.id.clone()); - } else { - let mut ag = ArgGroup::with_id(g.clone()); - ag.args.push(a.id.clone()); - self.groups.push(ag); + // Figure out implied settings + if a.is_set(ArgSettings::Last) { + // if an arg has `Last` set, we need to imply DontCollapseArgsInUsage so that args + // in the usage string don't get confused or left out. + self.settings.set(AppSettings::DontCollapseArgsInUsage); + self.settings.set(AppSettings::ContainsLast); + } + a._build(); + if a.short.is_none() && a.long.is_none() && a.index.is_none() { + a.index = Some(pos_counter); + pos_counter += 1; } } - // Figure out implied settings - if a.is_set(ArgSettings::Last) { - // if an arg has `Last` set, we need to imply DontCollapseArgsInUsage so that args - // in the usage string don't get confused or left out. - self.settings.set(AppSettings::DontCollapseArgsInUsage); - self.settings.set(AppSettings::ContainsLast); - } - a._build(); - if a.short.is_none() && a.long.is_none() && a.index.is_none() { - a.index = Some(pos_counter); - pos_counter += 1; - } + self.args._build(); + + #[cfg(debug_assertions)] + self::debug_asserts::assert_app(self); + self.settings.set(AppSettings::Built); + } else { + debug!("App::_build: already built"); } - - self.args._build(); - self.settings.set(AppSettings::Built); - - #[cfg(debug_assertions)] - self::debug_asserts::assert_app(self); } fn _panic_on_missing_help(&self, help_required_globally: bool) { @@ -2376,7 +2364,8 @@ impl<'help> App<'help> { two_elements_of(self.groups.iter().filter(|a| condition(a))) } - pub(crate) fn _propagate(&mut self, prop: Propagation) { + /// Propagate one and only one level. + pub(crate) fn _propagate(&mut self) { macro_rules! propagate_subcmd { ($_self:expr, $sc:expr) => {{ // We have to create a new scope in order to tell rustc the borrow of `sc` is @@ -2418,24 +2407,8 @@ impl<'help> App<'help> { debug!("App::_propagate:{}", self.name); - match prop { - Propagation::NextLevel | Propagation::Full => { - for sc in &mut self.subcommands { - propagate_subcmd!(self, sc); - if prop == Propagation::Full { - sc._propagate(prop.clone()); - } - } - } - Propagation::To(id) => { - let mut sc = self - .subcommands - .iter_mut() - .find(|sc| sc.id == id) - .expect(INTERNAL_ERROR_MSG); - propagate_subcmd!(self, sc); - } - Propagation::None => (), + for sc in &mut self.subcommands { + propagate_subcmd!(self, sc); } } diff --git a/src/build/app/tests.rs b/src/build/app/tests.rs index 388570c5..62cf2b46 100644 --- a/src/build/app/tests.rs +++ b/src/build/app/tests.rs @@ -1,4 +1,4 @@ -use crate::{build::app::Propagation, App, AppSettings}; +use crate::{App, AppSettings}; #[test] fn global_version() { @@ -6,7 +6,7 @@ fn global_version() { .setting(AppSettings::GlobalVersion) .version("1.1") .subcommand(App::new("sub1")); - app._propagate(Propagation::NextLevel); + app._propagate(); assert_eq!(app.subcommands[0].version, Some("1.1")); } @@ -15,7 +15,7 @@ fn global_setting() { let mut app = App::new("test") .global_setting(AppSettings::ColoredHelp) .subcommand(App::new("subcmd")); - app._propagate(Propagation::NextLevel); + app._propagate(); assert!(app .subcommands .iter() @@ -30,7 +30,7 @@ fn global_settings() { .global_setting(AppSettings::ColoredHelp) .global_setting(AppSettings::TrailingVarArg) .subcommand(App::new("subcmd")); - app._propagate(Propagation::NextLevel); + app._propagate(); assert!(app .subcommands .iter() diff --git a/src/build/usage_parser.rs b/src/build/usage_parser.rs index 2a248170..e5f07540 100644 --- a/src/build/usage_parser.rs +++ b/src/build/usage_parser.rs @@ -44,9 +44,11 @@ impl<'help> UsageParser<'help> { pub(crate) fn parse(mut self) -> Arg<'help> { debug!("UsageParser::parse"); - let mut arg = Arg::default(); - arg.disp_ord = 999; - arg.unified_ord = 999; + let mut arg = Arg { + disp_ord: 999, + unified_ord: 999, + ..Default::default() + }; loop { debug!("UsageParser::parse:iter: pos={}", self.pos); self.stop_at(token); diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 8b910279..638c08ac 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -6,7 +6,6 @@ use std::{ // Internal use crate::{ - build::app::Propagation, build::AppSettings as AS, build::{App, Arg, ArgSettings}, mkeymap::KeyType, @@ -865,9 +864,6 @@ impl<'help, 'app> Parser<'help, 'app> { help_help = true; break; // Maybe? } - if let Some(id) = sc.find_subcommand(cmd).map(|x| x.id.clone()) { - sc._propagate(Propagation::To(id)); - } if let Some(mut c) = sc.find_subcommand(cmd).cloned() { c._build(); @@ -975,11 +971,6 @@ impl<'help, 'app> Parser<'help, 'app> { } } - if let Some(x) = self.app.find_subcommand(sc_name) { - let id = x.id.clone(); - self.app._propagate(Propagation::To(id)); - } - if let Some(sc) = self.app.subcommands.iter_mut().find(|s| s.name == sc_name) { let mut sc_matcher = ArgMatcher::default(); // Display subcommand name, short and long in usage