-
Notifications
You must be signed in to change notification settings - Fork 640
Format custom help message with configured column width #413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2300,6 +2300,58 @@ format_description | |||||||||
| return result; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| String | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (5) If
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There may be long URLs in the help, so we need to handle words of very long length |
||||||||||
| format_custom_help | ||||||||||
| ( | ||||||||||
| const std::string& custom_help, | ||||||||||
| std::size_t start, | ||||||||||
| std::size_t allowed | ||||||||||
| ) | ||||||||||
| { | ||||||||||
| String result; | ||||||||||
| std::size_t count = 0; | ||||||||||
| std::size_t word_size = 0; | ||||||||||
|
|
||||||||||
| if(allowed <= start) | ||||||||||
| { | ||||||||||
| throw_or_mimic<exceptions::invalid_option_format>("Allowed column" | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (7) With current code, the exception will only get thrown when users use The exception should be thrown at the time of We should be throwing exceptions when users set an invalid configuration, not when they attempt to use that invalid config.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with that. |
||||||||||
| "width must be greater than start column width!"); | ||||||||||
|
Comment on lines
+2317
to
+2318
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (2) Typo
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| for (std::size_t i = 0; i < custom_help.length(); i++) | ||||||||||
| { | ||||||||||
| char c = custom_help[i]; | ||||||||||
|
|
||||||||||
| while (count < start) { | ||||||||||
| result.push_back(' '); | ||||||||||
| count++; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // record the start of a word | ||||||||||
| word_size = (std::isspace(c)) ? 0 : word_size + 1; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (3) Please use |
||||||||||
|
|
||||||||||
| result.push_back(c); | ||||||||||
|
|
||||||||||
| count = (c == '\n') ? 0 : count + 1; | ||||||||||
|
|
||||||||||
| if (count >= allowed) | ||||||||||
| { | ||||||||||
| // if we are in the middle of a word, backtrack until word_size is 0 | ||||||||||
|
|
||||||||||
| for (std::size_t c1 = 0; c1 < word_size; c1++) | ||||||||||
| { | ||||||||||
| result.pop_back(); | ||||||||||
| i--; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| result.push_back('\n'); | ||||||||||
| count = 0; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return result; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| } // namespace | ||||||||||
|
|
||||||||||
| inline | ||||||||||
|
|
@@ -2886,6 +2938,7 @@ Options::help(const std::vector<std::string>& help_groups, bool print_usage) con | |||||||||
| if (!m_custom_help.empty()) | ||||||||||
| { | ||||||||||
| result += " " + toLocalString(m_custom_help); | ||||||||||
| result = format_custom_help(result, OPTION_DESC_GAP, m_width); | ||||||||||
|
Comment on lines
2940
to
+2941
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (1) @amirsojoodi This will fail in compilation for
Please fix that and ensure that we compile with that flag.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jarro2783 We would need to fix the github CI, or we might cause regressions in the current code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am gonna give it a shot this weekend.
Comment on lines
2938
to
+2941
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (8) At this point, you are formatting the entire Since your function is named Right now even the main help is getting formatted. cxxopts::Options options("checker", "Test cxxopts with a very very very very very long program description");
options.custom_help(" 123456789 123456789 123456789 123456789 123456789");
options.add_options()
("h,help", "Print usage")
;
options.set_width(30);
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So either you should only format the custom help part, or use the function irrespective of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jarro2783 What do you think our output should be in this case cxxopts::Options options("checker", "Test cxxopts with a very very very very very long program description");
options.custom_help(" 123456789 123456789 123456789 123456789 123456789");
options.add_options()
("h,help", "Print usage");
options.set_width(30);(a) This (b) Or this |
||||||||||
| } | ||||||||||
|
|
||||||||||
| if (!m_positional.empty() && !m_positional_help.empty()) { | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1191,3 +1191,39 @@ TEST_CASE("No Options help", "[options]") | |||||||||||||||||||||||||||
| CHECK_NOTHROW(options.parse(argc, argv)); | ||||||||||||||||||||||||||||
| CHECK(options.help().find("test <posArg1>...<posArgN>") != std::string::npos); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| TEST_CASE("Format custom message with selected width", "[help]") | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (6) Test cases are lacking a lot of checks. Please ensure that they cover most of the issues above. Please also include a test for throwing |
||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| cxxopts::Options options("Custom message width", | ||||||||||||||||||||||||||||
| " - test custom message formatting width"); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| options.custom_help( | ||||||||||||||||||||||||||||
| "A very very long description that should be wrapped according to the " | ||||||||||||||||||||||||||||
| "specified width for help messages. This is to ensure that lines are not " | ||||||||||||||||||||||||||||
| "very long and remain readable. Just to make sure we have enough text here " | ||||||||||||||||||||||||||||
| "to trigger the wrapping functionality properly. Let's add a bit more text " | ||||||||||||||||||||||||||||
| "to be certain!"); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| options.set_width(60); | ||||||||||||||||||||||||||||
| const auto help = options.help(); | ||||||||||||||||||||||||||||
| CHECK( | ||||||||||||||||||||||||||||
| help.find( | ||||||||||||||||||||||||||||
| "Custom message width A very very long description that \n should " | ||||||||||||||||||||||||||||
| "be wrapped according to the specified width for \n help messages. " | ||||||||||||||||||||||||||||
| "This is to ensure that lines are not very \n long and remain " | ||||||||||||||||||||||||||||
| "readable. Just to make sure we have \n enough text here to trigger " | ||||||||||||||||||||||||||||
| "the wrapping functionality \n properly. Let's add a bit more text " | ||||||||||||||||||||||||||||
| "to be certain!") != std::string::npos); | ||||||||||||||||||||||||||||
|
Comment on lines
+1211
to
+1216
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (0) Suggestion - Defining it like this will make the tests more readable and clear
Suggested change
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll merge it with the formatting changes. |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| options.set_width(90); | ||||||||||||||||||||||||||||
| const auto help90 = options.help(); | ||||||||||||||||||||||||||||
| CHECK( | ||||||||||||||||||||||||||||
| help90.find( | ||||||||||||||||||||||||||||
| "Custom message width A very very long description that should be " | ||||||||||||||||||||||||||||
| "wrapped according to \n the specified width for help messages. " | ||||||||||||||||||||||||||||
| "This is to ensure that lines are not very long \n and remain " | ||||||||||||||||||||||||||||
| "readable. Just to make sure we have enough text here to trigger the " | ||||||||||||||||||||||||||||
| "wrapping \n functionality properly. Let's add a bit more text to " | ||||||||||||||||||||||||||||
| "be certain!") != | ||||||||||||||||||||||||||||
| std::string::npos); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(4) @amirsojoodi
the output sometimes contains extra unexpected lines that should not be appendedThe output appends extra spaces when user gave only newlines.
I think, this should ideally show something like this
But its actually this right now
This is almost unnoticeable to the user, but is a slightly unexepected behaviour in my opinion.