-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
Conditionally trim trailing periods of argument and option descriptions #1740
Conditionally trim trailing periods of argument and option descriptions #1740
Conversation
@microsoft-github-policy-service agree |
Thanks, I'll try to review this one over the weekend. |
Hello @TheTonttu, thank you for the contribution. My brain isn't fully functional today, however it looks like there is one more change required in this PR. Take one of the expectations, say I believe this line needs normalising too: Can you please check and see if you concur? If so, make the change and repush. |
PS. @TheTonttu I plan to review/merge this #1717 soon, so if it's before your next push, you'll need to rebase and update accordingly. |
Good catch, I'll make an update soon™. |
The change affected quite many test cases. |
It looks ok to me, /// <summary>
/// Gets or sets a value indicating whether a trailing period of a command description is trimmed in the help text.
/// </summary>
bool TrimTrailingPeriod { get; set; } Prior to your PR, the actual command description wouldn't ever be trimmed! I just want to triple check with one of our other maintainers over the weekend first... |
Note to self: The more I think about the idea of The trim only ever happens on text entered by the Spectre.Console application developer, and it's compile time text rather than dynamic somehow (ie. it's known in advance) - so why on earth do we have a property to offer to trim their explicitly entered text? If they want it trimmed, then simply don't use a period at the end. Furthermore, the trim is defaulting to I think I'm in favour of marking the property as obsolete and also changing its default to |
Changed the Should_Not_Trim_Description_Trailing_Period test case to output help with descriptions for command, argument, and option.
…u localizations Chinese (zh-Hans), Japanese (jp), and Korean (ko) localizations were not touched because I'm not sure about the punctuation rules for those languages. I think Chinese and Japanese mostly use 。(Ideographic Full Stop) for ending horizontal sentence. Modern Korean seems to use western punctuation marks.
6e87d41
to
33178a2
Compare
Yeah, the trailing period trimming really only makes sense if app developer wants to remove them from the built-in descriptions but it doesn't work with languages that use something else than period to end a sentence. Ideally there would just be convenient interface to customize the built-in descriptions. For now I rebased the feature branch to the latest main branch and added periods to the new Italian, Portuguese, and Russian help and version description localizations. I didn't touch the Chinese, Japanese and Korean localizations because I'm not familiar with the punctuation rules. As far as I know, Chinese and Japanese use 。(Ideographic full stop) for horizontal text and modern Korean uses western punctuation marks. |
Hello @TheTonttu, I've really thought about this PR and I would appreciate your own unbiased views, if you could take off the developer hat and wear a reviewer one for a moment. Your work is high quality and I have no concerns with it at all. I take issued with the existing 'trim trailing period' behaviour and implementation, but I have also come to the conclusion that your PR improves status quo - so should be merged. Let me explain my logic, please follow along and tell me your own take, if different. The original question really is - "What kind of behaviour do we want out of the box, specifically how do we handle the issue of trailing full stops for single and multiline text?" Visually, it's akin to whether we want the left or the right side of the following (illustrative) example: Current behaviour is trimming the trailing full stop (RHS), although not consistently (hence this PR). The top-level description wasn't being trimmed at all, which we added recently. I've looked at numerous GNU standard utilities and they seem to generally leave the trailing full stop on the description, removing the full stop on the options and argument descriptions, see: I don't believe this approach fully works for us, because My ideal solution Other considerations
Recommendation
|
My opinion: I think this should be configurable like it is now, but consistent. I want to keep this for scenarios where I don't have access to the actual command settings, and this is an actual scenario. At least give me a |
Thanks @patriksvensson, that was the guidance I was after. I'll merge as is, which certainly improves consistency. An |
Thank you @TheTonttu, very much appreciated. Please contribute again. |
Thank you @FrankRay78 for the contribution opportunity. 😄 The proposed For my unconventional use case, I am cramming all option details into the description, so that all the relevant information can be found in one place and in sync with the program version. There the original trailing full stop trimming was awkward because last sentence was missing the full stop. Of course generally verbose descriptions should be avoided if they don't add any value. In my (niche?) case it would be nice to have optional verbosity parameter ([b]rief/[v]erbose) for the help option in case user wants quick overview/reminder of the available command arguments and options instead of the full details. Although, I guess that sort of verbosity parameter is out of scope for this library and anyways further deviates away from all sorts of standards and conventions. |
Have a look at this @TheTonttu : It doesn't seem the idea of concise/full help is unconventional, rather it's use depends on the complexity of the command app. I plan to raise a new feature request for this. If it's open source, or if you can share somehow, it would be interesting to see your application's help output. |
Thanks, interesting site. I'll have to read it through thoroughly at some point. 🔍 The help descriptions I used are more or less just composed of a brief one-line description followed by more details. In my case the brief help could simply cut off after first line (break) and the full help would be the full text. Mock example:
|
fixes #1729
Changes
HelpProvider.TrimTrailingPeriod
property affects trailing period of argument and option descriptions in addition to command descriptions.Please upvote 👍 this pull request if you are interested in it.