Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Wrap --help output to 120 characters #7626

Merged
merged 9 commits into from
Jan 31, 2018
Merged

Wrap --help output to 120 characters #7626

merged 9 commits into from
Jan 31, 2018

Conversation

axelchalon
Copy link
Contributor

@axelchalon axelchalon commented Jan 18, 2018

The output of --help will now wrap at 100 characters (or to the terminal size if it's smaller than 100 characters). I used textwrap and term_size, which are two libraries also used by Clap.

textwrap doesn't support correctly wrapping strings with line breaks, so for now I removed the line breaks in the few help messages that had some, and made a PR on textwrap to add support.

Also Clap fixed some bugs on their latest version (dependency version was bumped yesterday by Marek) and added support for global arguments now natively propagating up and down (so that they can be placed anywhere in the command), and so I removed the workaround I had used. The code is a bit simpler now.

Output of parity --help: https://gist.github.com/axelchalon/d2aa66d26e19ea8f5484150af1a2ea62
Output of parity export state --help: https://gist.github.com/axelchalon/69d50f8406ede0354176b94c8939a192


Closes #7428

@axelchalon axelchalon added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 18, 2018
@axelchalon axelchalon changed the title Wrap --help output to 100 characters; update Clap dependency Wrap --help output to 100 characters Jan 18, 2018
@5chdn 5chdn added this to the 1.10 milestone Jan 19, 2018
@5chdn 5chdn added the B0-patch label Jan 22, 2018
@5chdn 5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018
@axelchalon
Copy link
Contributor Author

axelchalon commented Jan 23, 2018

Last commit closes #7428 and denies multiple values from being separated by spaces any longer, as this conflicts with values being able to start with hyphens ( --allow-ips -10.0.0.0/8; --password file1 file2 --other-option used to be interpreted as --password with three values file1 file2 --other-option). Multiple values now need to be separated by a comma: --password file1,file2 --other-option.

@5chdn 5chdn added the P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Jan 24, 2018
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're able to properly detect the terminal width why force 100 characters?

help.push_str($group_name); help.push_str(":\n");

$(
help.push_str(&format!("\t{}\n\t\t{}\n", $flag_usage, $flag_help));
help.push_str(&format!(" {}\n{}\n", $flag_usage,
indent(fill($flag_help, term_width-8).as_ref()," ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you extract this into a variable (const INDENT_PREFIX: &str = "        ";). And then you can replace the 8 with INDENT_PREFIX.len().

@5chdn 5chdn added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 24, 2018
@axelchalon
Copy link
Contributor Author

@andresilva I think because it's not very readable to have such long lines on wide screens #2913 (comment)

@5chdn
Copy link
Contributor

5chdn commented Jan 25, 2018

We can still use the terminal width, but limit it to 120 chars max.

@andresilva
Copy link
Contributor

@axelchalon makes sense. I don't really have strong opinions on what the maximum should be (100 vs 120), maybe whatever looks better on wide screens.

https://gist.github.com/axelchalon/d2aa66d26e19ea8f5484150af1a2ea62#file-gistfile1-txt-L16
Maybe this should be indented? Not sure if it's easy to do it or not.

Overall the code and the resulting help output LGTM.

@axelchalon
Copy link
Contributor Author

@5chdn currently, terminal width is used but limited to 100 characters. I don't have an opinion on 100 vs 120 characters either; whichever is preferred!

@andresilva yes, you're right. last commit fixes this; I updated the gists

@5chdn
Copy link
Contributor

5chdn commented Jan 26, 2018

Just played around with my terminal, 120 is pretty readable on a wide-screen. I would not go above that, and smaller terminals should just auto-detect the width. With high-resolution screens available I think it's fine to go for 120 over 100. 100 or even 80 (IBM punch card style) might be too narrow.

@axelchalon
Copy link
Contributor Author

👍 Gists are updated

@axelchalon axelchalon changed the title Wrap --help output to 100 characters Wrap --help output to 120 characters Jan 26, 2018
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 28, 2018
@tomusdrw
Copy link
Collaborator

Why B0-patch? Does it fix any bugs?

@5chdn
Copy link
Contributor

5chdn commented Jan 29, 2018

@tomusdrw no I wanted this in for 1.9 but we missed that train. nvm.

edit, err, yes it fixes the --password flag #7428

@5chdn 5chdn removed the P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Jan 29, 2018
@5chdn 5chdn merged commit fee88d0 into master Jan 31, 2018
@5chdn 5chdn deleted the cli-remove-workarounds branch January 31, 2018 08:50
andresilva pushed a commit that referenced this pull request Jan 31, 2018
* Update Clap dependency and remove workarounds

* WIP

* Remove line breaks in help messages for now

* Multiple values can only be separated by commas (closes #7428)

* Grumbles; refactor repeating code; add constant

* Use a single Wrapper rather than allocate a new one for each call

* Wrap --help to 120 characters rather than 100 characters
@andresilva andresilva mentioned this pull request Jan 31, 2018
andresilva pushed a commit that referenced this pull request Jan 31, 2018
* Update Clap dependency and remove workarounds

* WIP

* Remove line breaks in help messages for now

* Multiple values can only be separated by commas (closes #7428)

* Grumbles; refactor repeating code; add constant

* Use a single Wrapper rather than allocate a new one for each call

* Wrap --help to 120 characters rather than 100 characters
@andresilva andresilva mentioned this pull request Jan 31, 2018
andresilva pushed a commit that referenced this pull request Jan 31, 2018
* Update Clap dependency and remove workarounds

* WIP

* Remove line breaks in help messages for now

* Multiple values can only be separated by commas (closes #7428)

* Grumbles; refactor repeating code; add constant

* Use a single Wrapper rather than allocate a new one for each call

* Wrap --help to 120 characters rather than 100 characters
5chdn pushed a commit that referenced this pull request Jan 31, 2018
* Filter-out nodes.json (#7716)

* Filter-out nodes.json

* network: sort node table nodes by failure ratio

* network: fix node table tests

* network: fit node failure percentage into buckets of 5%

* network: consider number of attempts in sorting of node table

* network: fix node table grumbles

* Fix client not being dropped on shutdown (#7695)

* parity: wait for client to drop on shutdown

* parity: fix grumbles in shutdown wait

* parity: increase shutdown timeouts

* Wrap --help output to 120 characters (#7626)

* Update Clap dependency and remove workarounds

* WIP

* Remove line breaks in help messages for now

* Multiple values can only be separated by commas (closes #7428)

* Grumbles; refactor repeating code; add constant

* Use a single Wrapper rather than allocate a new one for each call

* Wrap --help to 120 characters rather than 100 characters
5chdn pushed a commit that referenced this pull request Jan 31, 2018
* Filter-out nodes.json (#7716)

* Filter-out nodes.json

* network: sort node table nodes by failure ratio

* network: fix node table tests

* network: fit node failure percentage into buckets of 5%

* network: consider number of attempts in sorting of node table

* network: fix node table grumbles

* Fix client not being dropped on shutdown (#7695)

* parity: wait for client to drop on shutdown

* parity: fix grumbles in shutdown wait

* parity: increase shutdown timeouts

* Wrap --help output to 120 characters (#7626)

* Update Clap dependency and remove workarounds

* WIP

* Remove line breaks in help messages for now

* Multiple values can only be separated by commas (closes #7428)

* Grumbles; refactor repeating code; add constant

* Use a single Wrapper rather than allocate a new one for each call

* Wrap --help to 120 characters rather than 100 characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using a password file
4 participants