From 946b76331294365eb624a2fc5aba3c16b7feef28 Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Fri, 19 May 2017 12:38:52 +0200 Subject: [PATCH 1/3] tests: benchmark building ripgrep help text The ripgrep benchmarks work with lots of options and have really long help texts. This makes them a good fit for judging the overall time it takes to construct and format the help text. --- benches/05_ripgrep.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/benches/05_ripgrep.rs b/benches/05_ripgrep.rs index 42a43c92284..48c0819bd24 100644 --- a/benches/05_ripgrep.rs +++ b/benches/05_ripgrep.rs @@ -14,6 +14,7 @@ use clap::{App, AppSettings, Arg, ArgSettings}; use test::Bencher; use std::collections::HashMap; +use std::io::Cursor; #[bench] fn build_app_short(b: &mut Bencher) { b.iter(|| app_short()); } @@ -21,6 +22,18 @@ fn build_app_short(b: &mut Bencher) { b.iter(|| app_short()); } #[bench] fn build_app_long(b: &mut Bencher) { b.iter(|| app_long()); } +#[bench] +fn build_help_short(b: &mut Bencher) { + let app = app_short(); + b.iter(|| build_help(&app)); +} + +#[bench] +fn build_help_long(b: &mut Bencher) { + let app = app_long(); + b.iter(|| build_help(&app)); +} + #[bench] fn parse_clean(b: &mut Bencher) { b.iter(|| app_short().get_matches_from(vec!["rg", "pat"])); } @@ -304,6 +317,16 @@ pub fn app_short() -> App<'static, 'static> { app(false, |k| USAGES[k].short) } /// Build a clap application with long help strings. pub fn app_long() -> App<'static, 'static> { app(true, |k| USAGES[k].long) } + +/// Build the help text of an application. +fn build_help(app: &App) -> String { + let mut buf = Cursor::new(Vec::with_capacity(50)); + app.write_help(&mut buf).unwrap(); + let content = buf.into_inner(); + String::from_utf8(content).unwrap() +} + + /// Build a clap application parameterized by usage strings. /// /// The function given should take a clap argument name and return a help From d0699aa214c8d826cb437a53a45f9bb80e01f610 Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Sun, 29 Jan 2017 21:38:30 +0100 Subject: [PATCH 2/3] tests: add wrap_help test with significant whitepace This adds a test for the issue #617 about keeping whitespace intact in manually aligned text. --- tests/help.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/help.rs b/tests/help.rs index 9bbb4fd3528..ff51311403e 100644 --- a/tests/help.rs +++ b/tests/help.rs @@ -241,6 +241,22 @@ FLAGS: (Defaults to something) -V, --version Prints version information"; +static WRAPPING_NEWLINE_CHARS: &'static str = "ctest 0.1 + +USAGE: + ctest [mode] + +FLAGS: + -h, --help Prints help information + -V, --version Prints version information + +ARGS: + x, max, maximum 20 characters, contains + symbols. + l, long Copy-friendly, + 14 characters, contains symbols. + m, med, medium Copy-friendly, 8 + characters, contains symbols."; static ISSUE_688: &'static str = "ctest 0.1 @@ -663,6 +679,18 @@ fn final_word_wrapping() { assert!(test::compare_output(app, "ctest --help", FINAL_WORD_WRAPPING, false)); } +#[test] +fn wrapping_newline_chars() { + let app = App::new("ctest") + .version("0.1") + .set_term_width(60) + .arg(Arg::with_name("mode") + .help("x, max, maximum 20 characters, contains symbols.{n}\ + l, long Copy-friendly, 14 characters, contains symbols.{n}\ + m, med, medium Copy-friendly, 8 characters, contains symbols.{n}")); + assert!(test::compare_output(app, "ctest --help", WRAPPING_NEWLINE_CHARS, false)); +} + #[test] fn old_newline_chars() { let app = App::new("ctest") From 12282e10510754ff5f455e65d638648df3643ee9 Mon Sep 17 00:00:00 2001 From: Martin Geisler Date: Sat, 28 Jan 2017 12:42:09 +0100 Subject: [PATCH 3/3] feat: use textwrap crate for wrapping help texts The textwrap crate uses a simpler linear-time algorithm for wrapping the text. The current algorithm in wrap_help uses several O(n) calls to String::insert and String::remove, which makes it potentially quadratic in complexity. Comparing the 05_ripgrep benchmark at commits textwrap~2 and textwrap gives this result on my machine: name before ns/iter after ns/iter diff ns/iter diff % build_app_long 22,101 21,099 -1,002 -4.53% build_app_short 22,138 21,205 -933 -4.21% build_help_long 514,265 284,467 -229,798 -44.68% build_help_short 85,720 85,693 -27 -0.03% parse_clean 23,471 22,859 -612 -2.61% parse_complex 29,535 28,919 -616 -2.09% parse_lots 422,815 414,577 -8,238 -1.95% As part of this commit, the wrapping_newline_chars test was updated. The old algorithm had a subtle bug where it would break lines too early. That is, it wrapped the text like ARGS: x, max, maximum 20 characters, contains symbols. l, long Copy-friendly, 14 characters, contains symbols. m, med, medium Copy-friendly, 8 characters, contains symbols."; when it should really have wrapped it like ARGS: x, max, maximum 20 characters, contains symbols. l, long Copy-friendly, 14 characters, contains symbols. m, med, medium Copy-friendly, 8 characters, contains symbols."; Notice how the word "14" was incorrectly moved to the next line. There is clearly room for the word on the line with the "l, long" option since there is room for "contains" just above it. I'm not sure why this is, but the algorithm in textwrap handles this case correctly. --- Cargo.toml | 1 + src/app/help.rs | 46 +++++++--------------------------------------- src/lib.rs | 1 + tests/help.rs | 4 ++-- 4 files changed, 11 insertions(+), 41 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 2c0a0dcc194..c9c3ea85e56 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ bitflags = "0.8.0" vec_map = "0.8" unicode-width = "0.1.4" unicode-segmentation = "~1.1.0" # 1.2.0 requires Rust 1.13.0 +textwrap = "0.6.0" strsim = { version = "0.6.0", optional = true } ansi_term = { version = "0.9.0", optional = true } term_size = { version = "0.3.0", optional = true } diff --git a/src/app/help.rs b/src/app/help.rs index f8ca33486cf..b2f749f547d 100644 --- a/src/app/help.rs +++ b/src/app/help.rs @@ -17,7 +17,7 @@ use app::usage; use unicode_width::UnicodeWidthStr; #[cfg(feature = "wrap_help")] use term_size; -use unicode_segmentation::UnicodeSegmentation; +use textwrap; use vec_map::VecMap; #[cfg(not(feature = "wrap_help"))] @@ -955,45 +955,13 @@ impl<'a> Help<'a> { } fn wrap_help(help: &mut String, longest_w: usize, avail_chars: usize) { - debugln!("Help::wrap_help: longest_w={}, avail_chars={}", - longest_w, - avail_chars); - debug!("Help::wrap_help: Enough space to wrap..."); + // Keep previous behavior of not wrapping at all if one of the + // words would overflow the line. if longest_w < avail_chars { - sdebugln!("Yes"); - let mut prev_space = 0; - let mut j = 0; - for (idx, g) in (&*help.clone()).grapheme_indices(true) { - debugln!("Help::wrap_help:iter: idx={}, g={}", idx, g); - if g == "\n" { - debugln!("Help::wrap_help:iter: Newline found..."); - debugln!("Help::wrap_help:iter: Still space...{:?}", - str_width(&help[j..idx]) < avail_chars); - if str_width(&help[j..idx]) < avail_chars { - j = idx; - continue; - } - } else if g != " " { - if idx != help.len() - 1 || str_width(&help[j..idx]) < avail_chars { - continue; - } - debugln!("Help::wrap_help:iter: Reached the end of the line and we're over..."); - } else if str_width(&help[j..idx]) <= avail_chars { - debugln!("Help::wrap_help:iter: Space found with room..."); - prev_space = idx; - continue; - } - debugln!("Help::wrap_help:iter: Adding Newline..."); - j = prev_space; - debugln!("Help::wrap_help:iter: prev_space={}, j={}", prev_space, j); - debugln!("Help::wrap_help:iter: Removing...{}", j); - debugln!("Help::wrap_help:iter: Char at {}: {:?}", j, &help[j..j + 1]); - help.remove(j); - help.insert(j, '\n'); - prev_space = idx; - } - } else { - sdebugln!("No"); + *help = help.lines() + .map(|line| textwrap::fill(line, avail_chars)) + .collect::>() + .join("\n"); } } diff --git a/src/lib.rs b/src/lib.rs index 7c3310ca1e7..2c212bf55b2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -541,6 +541,7 @@ extern crate bitflags; extern crate vec_map; #[cfg(feature = "wrap_help")] extern crate term_size; +extern crate textwrap; extern crate unicode_segmentation; #[cfg(feature = "color")] extern crate atty; diff --git a/tests/help.rs b/tests/help.rs index ff51311403e..b00ee048ac0 100644 --- a/tests/help.rs +++ b/tests/help.rs @@ -253,8 +253,8 @@ FLAGS: ARGS: x, max, maximum 20 characters, contains symbols. - l, long Copy-friendly, - 14 characters, contains symbols. + l, long Copy-friendly, 14 + characters, contains symbols. m, med, medium Copy-friendly, 8 characters, contains symbols.";