-
Notifications
You must be signed in to change notification settings - Fork 633
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?
Conversation
|
Can you write a unit test for this? |
|
@amirsojoodi Are you still working on this? |
|
@lazysegtree I've forgotten about this. Give me some time, and I'll update |
a638577 to
3945f84
Compare
| "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); |
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.
(0) Suggestion - Defining it like this will make the tests more readable and clear
Same for below
| "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); | |
| "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); |
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.
I'll merge it with the formatting changes.
|
While testing this I observed crash when there is a word that is bigger than the parameter passed to set_width. I am gonna do a more thorough review and testing. |
| result += " " + toLocalString(m_custom_help); | ||
| result = format_custom_help(result, OPTION_DESC_GAP, m_width); |
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.
(1) @amirsojoodi This will fail in compilation for CXXOPTS_USE_UNICODE flag
For that we flag, we have using String = icu::UnicodeString;, and hence result here, will be of type icu::UnicodeString
format_custom_help only accepts std::string and wont allow icu::UnicodeString
Please fix that and ensure that we compile with that flag.
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.
@jarro2783 We would need to fix the github CI, or we might cause regressions in the current code.
I hope we do test the compilation with CXXOPTS_USE_UNICODE in our CI
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.
I am gonna give it a shot this weekend.
| throw_or_mimic<exceptions::invalid_option_format>("Allowed column" | ||
| "width must be greater than start column width!"); |
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.
(2) Typo
Missing concatanation of space in the error message
Should be
| throw_or_mimic<exceptions::invalid_option_format>("Allowed column" | |
| "width must be greater than start column width!"); | |
| throw_or_mimic<exceptions::invalid_option_format>("Allowed column " | |
| "width must be greater than start column width!"); |
| } | ||
|
|
||
| // record the start of a word | ||
| word_size = (std::isspace(c)) ? 0 : word_size + 1; |
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.
(3) Please use bool isspace( CharT ch, const locale& loc )
Elsewhere in the library, we use locale specific checks. For example, check the usage of isalnum
| return result; | ||
| } | ||
|
|
||
| String |
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 appended
The output appends extra spaces when user gave only newlines.
options.custom_help("This is a multiline custom help\nLine2\n\nLine4");
I think, this should ideally show something like this
3 │ ····checker·This·is·a·multiline·custom·help␊
4 │ ··Line2␊
5 │ ␊
6 │ ··Line4␊
7 │ ␊
But its actually this right now
➜ ~/Workspace/Other_proj/cxxopts-amir/include git:(patch-2) ✗ [9:02:28] g++ main.cpp && ./a.out -h | bat --show-all --no-pager
───────┬───────────────────────────────────────────────────────────────────────────────────────────────
│ STDIN
───────┼───────────────────────────────────────────────────────────────────────────────────────────────
1 │ ··Test·cxxopts␊
2 │ ··Usage:␊
3 │ ····checker·This·is·a·multiline·custom·help␊
4 │ ··Line2␊
5 │ ··␊
6 │ ··Line4␊
7 │ ␊
8 │ ··-d,·--debug2·······01234567890123456789␊
9 │ ··-i,·--integer·arg··Int·param␊
10 │ ··-f,·--file·arg·····File·name␊
11 │ ··-v,·--verbose······Verbose·output␊
12 │ ··-h,·--help·········Print·usage␊
13 │ ␊
───────┴───────────────────────────────────────────────────────────────────────────────────────────────
➜ ~/Workspace/Other_proj/cxxopts-amir/include git:(patch-2) ✗ [9:03:01]
This is almost unnoticeable to the user, but is a slightly unexepected behaviour in my opinion.
| return result; | ||
| } | ||
|
|
||
| String |
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.
(5) If options.set_width is called with a low value like 10, and there is a word of greater length. THe program gets stuck in infinite loop.
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.
There may be long URLs in the help, so we need to handle words of very long length
| CHECK(options.help().find("test <posArg1>...<posArgN>") != std::string::npos); | ||
| } | ||
|
|
||
| TEST_CASE("Format custom message with selected width", "[help]") |
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.
(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 exceptions::invalid_option_format on setting too less width.
|
|
||
| if(allowed <= start) | ||
| { | ||
| throw_or_mimic<exceptions::invalid_option_format>("Allowed column" |
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.
(7) With current code, the exception will only get thrown when users use -h, while the trigger was the user setting wrong value in options.set_width
The exception should be thrown at the time of set_width.
We should be throwing exceptions when users set an invalid configuration, not when they attempt to use that invalid config.
What do you think @jarro2783
# code with `options.set_width(1);` in main.cpp.
# If users don't test -h, they will not know of the misconfig.
➜ ~/Workspace/Other_proj/cxxopts-amir/include git:(patch-2) ✗ [12:50:12] g++ main.cpp && ./a.out -h
start
Invalid option format 'Allowed columnwidth must be greater than start column width!'
➜ ~/Workspace/Other_proj/cxxopts-amir/include git:(patch-2) ✗ [12:54:39] g++ main.cpp && ./a.out
start
help = false (default)
verbose = false (default)
debug2 = false (default)
➜ ~/Workspace/Other_proj/cxxopts-amir/include git:(patch-2) ✗ [12:54:44]
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.
@amirsojoodi Can you please fix comment 0-7. If some of them are not valid, and need further discussion, let me know.
| if (!m_custom_help.empty()) | ||
| { | ||
| result += " " + toLocalString(m_custom_help); | ||
| result = format_custom_help(result, OPTION_DESC_GAP, m_width); |
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.
(8) At this point, you are formatting the entire results that also contains Main program help -> m_help_string .
Since your function is named format_custom_help, I think that is not the intention here. You only intend to format the custom help part.
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);➜ ~/Workspace/Other_proj/cxxopts-amir/include git:(patch-2) ✗ [9:12:01] g++ main.cpp && ./a.out -h | bat --show-all --no-pager
───────┬───────────────────────────────────────────────────────────────────────────────────────────────
│ STDIN
───────┼───────────────────────────────────────────────────────────────────────────────────────────────
1 │ ··Test·cxxopts·with·a·very·␊
2 │ ··very·very·very·very·long·␊
3 │ ··program·description␊
4 │ ··Usage:␊
5 │ ····checker··123456789·␊
6 │ ··123456789·123456789·␊
7 │ ··123456789·123456789␊
8 │ ␊
9 │ ··-h,·--help··Print·usage␊
10 │ ␊
───────┴───────────────────────────────────────────────────────────────────────────────────────────────
➜ ~/Workspace/Other_proj/cxxopts-amir/include git:(patch-2) ✗ [9:14:48]
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.
So either you should only format the custom help part, or use the function irrespective of m_custom_help being empty or not, and name it to wrap_help or something generic.
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.
@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
1 │ ··Test·cxxopts·with·a·very·␊
2 │ ··very·very·very·very·long·␊
3 │ ··program·description␊
4 │ ··Usage:␊
5 │ ····checker··123456789·␊
6 │ ··123456789·123456789·␊
7 │ ··123456789·123456789␊
8 │ ␊
9 │ ··-h,·--help··Print·usage␊
10 │ ␊
(b) Or this
1 │ Test·cxxopts·with·a·very·very·very·very·very·long·program·description␊
2 │ Usage:␊
3 │ ··checker····123456789·123456789·␊
4 │ ··123456789·123456789·␊
5 │ ··123456789␊
6 │ ␊
7 │ ··-h,·--help··Print·usage␊
8 │ ␊
|
I think it would be more intuitive that all of the help gets formatted when you call |
|
If you rebase on master we will have working CI now. |
Wrap the custom help message according to the column width.
This change is