Skip to content
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

Conversation

TheTonttu
Copy link
Contributor

fixes #1729

  • I have read the Contribution Guidelines
  • I have commented on the issue above and discussed the intended changes
  • A maintainer has signed off on the changes and the issue was assigned to me
  • All newly added code is adequately covered by tests
  • All existing tests are still running without errors
  • The documentation was modified to reflect the changes OR no documentation changes are required.

Changes

  • HelpProvider.TrimTrailingPeriod property affects trailing period of argument and option descriptions in addition to command descriptions.
  • Related test case output includes argument and option description in addition to command description.
    • The test case (Should_Not_Trim_Description_Trailing_Period) expectation text (Description_No_Trailing_Period) is a bit confusing/conflicting but I didn't touch it to keep changes minimal.
  • Localized print help and print version descriptions include trailing period.
    • Default help output stays the same but when the trimming is disabled the period is added to the output.

Please upvote 👍 this pull request if you are interested in it.

@TheTonttu
Copy link
Contributor Author

@microsoft-github-policy-service agree

@FrankRay78 FrankRay78 self-requested a review January 24, 2025 20:45
@FrankRay78 FrankRay78 added the area-CLI Command-Line Interface label Jan 24, 2025
@FrankRay78 FrankRay78 added this to the 0.50 milestone Jan 24, 2025
@FrankRay78
Copy link
Contributor

Thanks, I'll try to review this one over the weekend.

@FrankRay78
Copy link
Contributor

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 Default_Without_Args_Additional.Output_EN.verified.txt, and it has the following inconsistent line endings for the description:

image

I believe this line needs normalising too:

image

Can you please check and see if you concur? If so, make the change and repush.

@FrankRay78
Copy link
Contributor

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.

@TheTonttu
Copy link
Contributor Author

Good catch, I'll make an update soon™.

@TheTonttu
Copy link
Contributor Author

The change affected quite many test cases.

@FrankRay78
Copy link
Contributor

FrankRay78 commented Jan 31, 2025

The change affected quite many test cases.

It looks ok to me, ICommandAppSettings does have the following description (in main, prior to your PR):

    /// <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...

@FrankRay78
Copy link
Contributor

Note to self: The more I think about the idea of TrimTrailingPeriod, the more I dislike it and the more I want to remove it.

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 true, so new users of Spectre.Console are faced with help displaying their own text but with trailing periods trimmed, which likely isn't obvious why and so seems strange default behaviour.

I think I'm in favour of marking the property as obsolete and also changing its default to false, which is hardly a breaking change. I need to give it a little more thought and look back through the git history.

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.
@TheTonttu TheTonttu force-pushed the fix/inconsistent-preserved-trailing-periods branch from 6e87d41 to 33178a2 Compare February 2, 2025 08:29
@TheTonttu
Copy link
Contributor Author

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.

@FrankRay78
Copy link
Contributor

FrankRay78 commented Feb 11, 2025

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:

image

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:

image

I don't believe this approach fully works for us, because --help at the root command will show commands and their descriptions (trimmed), but then <command> --help would display the description untrimmed (seems odd).

My ideal solution
To have never done any trimming, to have included resource strings without a period, to have a property called AddTrailingStopToResourceDescription, default the property to false, let the developer specify their own descriptions with trailing stops (should they set it to true).

Other considerations

  • Existing behaviour has been in place for a long time
  • We can break it, if we wish
  • Inconsistent preserving of trailing period in help descriptions #1729 is valid, this PR is a definite improvement
  • We shouldn't take no action
  • I see only one issue with the functionality in this PR; given a multiline description, then the default behaviour of trimming trailing stop will make that look odd (workaround: set trimming to false, specify a multiline description with a trailing stop, include trailing stops on all other descriptions - this would then play nicely with built-in --help and --version descriptions that include the trailing stop)

Recommendation
Take one of the following actions:

  1. Merge this PR as an improvement over status quo; or
  2. Implement the ideal solution above (instead of 1), or
  3. Merge with a view to implementing the ideal solution at some later point, if ever

@patriksvensson
Copy link
Contributor

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 IDescriptionFormatter or similar if we don't want to keep an explicit setting.

@FrankRay78
Copy link
Contributor

Thanks @patriksvensson, that was the guidance I was after. I'll merge as is, which certainly improves consistency. An IDescriptionFormatter approach can be slayed another day.

@FrankRay78 FrankRay78 merged commit 11a320c into spectreconsole:main Feb 11, 2025
3 checks passed
@FrankRay78
Copy link
Contributor

Thank you @TheTonttu, very much appreciated. Please contribute again.

@TheTonttu
Copy link
Contributor Author

Thank you @FrankRay78 for the contribution opportunity. 😄

The proposed IDescriptionFormatter would probably be nice replacement in the future for the current description formatting related help settings.

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.

@TheTonttu TheTonttu deleted the fix/inconsistent-preserved-trailing-periods branch February 14, 2025 16:59
@FrankRay78
Copy link
Contributor

Have a look at this @TheTonttu :

image

Ref: https://clig.dev/#help

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.

@TheTonttu
Copy link
Contributor Author

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:

DESCRIPTION:
Brief description of the example command.

More details about the command. Donec hendrerit nibh in augue sodales elementum. Nunc consequat tortor rhoncus,
consequat tortor eget, vestibulum orci. Cras hendrerit luctus iaculis. Vivamus non commodo ipsum. Nullam tortor
nisi, sodales eget sem ac, pulvinar mollis nisl. Etiam viverra lectus at sapien scelerisque iaculis. Donec 
rhoncus lacinia massa vel suscipit. Vestibulum varius, dolor quis mattis placerat, lorem tellus volutpat justo,
eu volutpat lorem lorem quis quam.

USAGE:
    CliVerboseHelpExample.dll example [OPTIONS]

EXAMPLES:
    CliVerboseHelpExample.dll example --choice Choice3 --lorem-ipsum Vitae

OPTIONS:
                                      DEFAULT                                                                  
    -h, --help                                     Prints help information.                                    
    -c, --choice <CHOICE>             Choice1      Choice that changes something.                              
                                                                                                               
                                                   [Choices]                                                   
                                                   Choice1 = Does something...                                 
                                                   Choice2 = Does something different...                       
                                                   Choice3 = Does something completely different...            
                                                                                                               
                                                   Fusce pretium pellentesque lacinia. Sed ut diam enim. Duis  
                                                   vehicula est fringilla purus condimentum fermentum. Sed     
                                                   pharetra mi justo, sit amet convallis urna pulvinar ac. Cras
                                                   eget pretium sapien. Vestibulum aliquam ultrices augue,     
                                                   placerat molestie libero facilisis sit amet. Morbi non felis
                                                   faucibus, accumsan nulla a, commodo orci. Nullam lectus     
                                                   nibh, maximus a elit at, interdum faucibus ex. Maecenas nec 
                                                   purus a diam sodales venenatis. Quisque malesuada magna     
                                                   massa, eget fringilla quam mattis quis. Pellentesque        
                                                   facilisis maximus est. Pellentesque vel sodales libero.     
                                                                                                               
                                                   Nulla a pretium ante. Nulla facilisi. Aliquam ut facilisis  
                                                   nisi. In posuere libero magna, sed pulvinar mauris dapibus  
                                                   ut. Curabitur tortor ipsum, scelerisque eget aliquam id,    
                                                   posuere interdum erat. Nunc id ante bibendum, blandit mi et,
                                                   viverra purus. Aliquam vehicula eu tellus sed finibus.      
                                                   Integer sed quam elementum, ultrices urna quis, cursus dui. 
                                                   Cras orci dui, mollis sit amet volutpat vitae, finibus nec  
                                                   ante. Integer at felis vel augue molestie consequat et vitae
                                                   felis. Donec ut ultricies nulla. Ut eget porttitor nunc.    
                                                   Maecenas dapibus consectetur ligula, eget malesuada nisl    
                                                   venenatis id. Suspendisse nec purus blandit libero luctus   
                                                   bibendum.                                                   
    -l, --lorem-ipsum <LOREMIPSUM>    Elementum    Lorem ipsum dolor sit amet, consectetur adipiscing elit.    
                                                                                                               
                                                   Cras tincidunt, arcu vitae congue mattis, magna lectus      
                                                   dignissim ex, et faucibus lorem lorem sed leo. Etiam velit  
                                                   dolor, eleifend placerat lacus non, posuere convallis nibh. 
                                                   Sed nec laoreet sapien. Ut purus elit, porta vel dignissim  
                                                   ac, ornare quis purus. Nunc at nibh at arcu ultricies       
                                                   lobortis a ut arcu. Integer libero mi, lobortis non suscipit
                                                   sit amet, tincidunt quis est. Pellentesque sapien libero,   
                                                   placerat quis sapien eu, aliquet pulvinar tellus. Praesent  
                                                   ac dui in ante aliquam pharetra id ultrices quam. Aliquam   
                                                   erat volutpat. Suspendisse eget volutpat dui. Sed ut rutrum 
                                                   odio.                                                       
                                                                                                               
                                                   Nam ac quam imperdiet, cursus nisi non, dignissim metus.    
                                                   Vestibulum vitae leo a metus gravida dictum at ac enim.     
                                                   Suspendisse faucibus, urna quis viverra rhoncus, leo nisl   
                                                   laoreet neque, tincidunt interdum dui purus id urna. Ut     
                                                   vitae odio cursus, fermentum nunc sed, semper eros. Donec   
                                                   tempus dignissim erat sit amet lacinia. Vivamus ac nunc     
                                                   ultrices, malesuada lacus nec, vestibulum neque. Sed eget   
                                                   pretium erat.                                               
    -m, --more <MORE>                              Imagine 10 or more additional options here...               

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CLI Command-Line Interface
Projects
Status: PR 📬
Development

Successfully merging this pull request may close these issues.

Inconsistent preserving of trailing period in help descriptions
3 participants