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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TheTonttu
Copy link

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
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
Author

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

@TheTonttu
Copy link
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
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.

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
2 participants