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

Consider a new localization design #1915

Open
Tracked by #1891
jonsequitur opened this issue Nov 2, 2022 · 6 comments
Open
Tracked by #1891

Consider a new localization design #1915

jonsequitur opened this issue Nov 2, 2022 · 6 comments

Comments

@jonsequitur
Copy link
Contributor

jonsequitur commented Nov 2, 2022

    // A) I don't know of another area that exposes their localization resources
    // B) All of the methods need verbs; or to instead be properties.
    public class LocalizationResources 
@KalleOlaviNiemitalo
Copy link

The current system can be used for these scenarios

  1. ensuring that custom parsers or validators output the same error messages as built-in ones used in the same application
  2. perhaps also for adding localisation for languages that are not natively supported by System.CommandLine, but I haven't yet tried that in practice

These are for user-visible strings, not for Exception.Message which often just goes to a log for server operators.

DateFormatInfo etc. classes also expose localisation but those have writable properties.

Microsoft.Extensions.Localization has IStringLocalizer, which could possibly be used instead of LocalizationResources, but then the key strings would have to be documented (and perhaps exposed as public constants), and it would become easier to make mistakes in parameterised strings.

@jonsequitur jonsequitur added this to the 2.0 GA milestone Nov 9, 2022
@adamsitnik
Copy link
Member

adamsitnik commented Feb 1, 2023

I took a look at the usage of LocalizationResources in dotnet/sdk as @jonsequitur told me that we added this type to satisfy the SDK requirements.

SDK defines a custom type CommandLineValidationMessages that derives from LocalizationResources. It overrides 14 out of 35 virtual methods. So not all strings are properly translated.

It defines it's own localizable strings: https://github.com/dotnet/sdk/tree/main/src/Cli/dotnet/CommandLineValidation/xlf I don't see why these translations should be part of SDK rather than System.CommandLine. Currently the translation work is duplicated and incomplete.

The instance of this custom type is passed to the configuration:

https://github.com/dotnet/sdk/blob/41ab0bc743b8b7effe145f0936a8fb3b48614060/src/Cli/dotnet/Parser.cs#L156

But it's not used everywhere it should:

https://github.com/dotnet/sdk/blob/41ab0bc743b8b7effe145f0936a8fb3b48614060/src/Cli/dotnet/Parser.cs#L245

https://github.com/dotnet/sdk/blob/41ab0bc743b8b7effe145f0936a8fb3b48614060/src/Tests/Microsoft.TemplateEngine.Cli.UnitTests/ParserTests/HelpTests.cs#L75

The APIs exposed by LocalizationResources are used in the SDK only by custom help builder:

https://github.com/dotnet/sdk/blob/main/src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.Help.cs#L319

And out of all 35 methods only 3 are used: HelpUsageTitle, HelpUsageOptions and HelpUsageCommand and they don't have a custom translation in CommandLineValidationMessages, moreover System.CommandLine currently is providing only English translation for these three texts ("Usage:", "[options]", "[command]").

I think we should do the following:

  1. Take translations defined in SDK and move them to System.CommandLine to avoid duplications.
  2. Ask the Community to translate the missing parts in System.CommandLine.
  3. Make LocalizationResources internal and sealed as System.* libraries that are part of base class libraries don't expose the possibility to provide custom translations for errors etc.
  4. Revisit the need for custom translations when working on HelpBuilder re-design.

@adamsitnik adamsitnik self-assigned this Feb 1, 2023
@KalleOlaviNiemitalo
Copy link

This gem in .NET SDK searches ParseResult.Errors for the localized UnrecognizedCommandOrArgument string:

https://github.com/dotnet/sdk/blob/98720693e228934405de152651b2e21390c3bfec/src/Cli/dotnet/ParseResultExtensions.cs#L41-L42

If LocalizationResources is made internal, then this will have to be implemented differently. Alternatives:

  • Copy all the UnrecognizedCommandOrArgument translations from System.CommandLine to .NET SDK and add a test to verify that they stay in sync -- both projects support exactly the same languages, and all translations match.
  • API change: add public enum ParseErrorKind { None, UnrecognizedCommandOrArgument, … }, a read-only property to class ParseError, and a parameter to SymbolResult.AddError in more flexible error reporting for SymbolResult #2033.
  • Assume that, if TreatUnmatchedTokensAsErrors is true, then each element of ParseResult.UnmatchedTokens results in one element of ParseResult.Errors; so you can just compare the number of elements and figure out whether there are any other errors.

@baronfel
Copy link
Member

baronfel commented Feb 1, 2023

@KalleOlaviNiemitalo has identified my shame 😄 We could probably remove this extension method if we got all of the commands in the .NET CLI to accurately describe their CLI surface areas (or at least allow unmatched tokens and provide an Argument to aggregate them), but given the different teams that contribute to the CLI it's hard to enforce that kind of uniformity. In lieu of that, having a more structured way to check if the only errors are unknown token errors would serve our need.

@jonsequitur
Copy link
Contributor Author

As far as I'm aware, we don't have another precedent for third-party translations for user-facing strings in a library like what we've enabled in System.CommandLine. We also don't have a process for accepting translations via OSS contributions. Figuring out the best way forward on this will take a bit longer and we don't want that decision to hold up the System.CommandLine GA, so the best course for now is to make these details internal, while adding out-of-the-box support the set of languages that are currently supported by the SDK.

The upside is that developers using System.CommandLine won't have to do anything special for their customers to see messages in those languages. This is an improvement over what we had in beta4.

The downside is that we don't yet have a story for additional languages.

@KalleOlaviNiemitalo
Copy link

Copy all the UnrecognizedCommandOrArgument translations from System.CommandLine to .NET SDK and add a test to verify that they stay in sync -- both projects support exactly the same languages, and all translations match.

Since dotnet/sdk#29746, .NET SDK assumes that the UnrecognizedCommandOrArgument translations match between System.CommandLine and CommandLineValidation.LocalizableStrings. However, they do not actually match. There are some differences in quotation marks and punctuation, and the es translations are very different: command-line-api, sdk. So, it appears that the proposed test was not implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants