[generator] Make warning and error messages localizable.

[jpobst]

Context: dotnet/android@0342fe5

Creates the infrastructure to make the Java.Interop repository localizable, and rebases generator to work on this infrastructure. Future PR's will make additional assemblies work on this infrastructure.

Approach

Creates a single Java.Interop.Localization.dll assembly that will be referenced by all localizable assemblies. This approach allows us to have a single set of .xlf files to worry about providing to translators, instead of a set for generator, class-parse, ApiXmlAdjuster, etc.

The downside to this approach is that if you only wanted to ship generator.exe you would be shipping the messages for the entire repo. Given that our use case is to ship all tools together, and that we won't have a ton of messages, this seems like an acceptable tradeoff. (generator contains the most messages, and with this PR Java.Interop.Localization.dll compiles to ~9KB.)

Currently we only plan to localize the tools used on desktop to build bindings. If we localize assemblies that run on device in the future, we would most likely make a separate "run-time" set, as we wouldn't want to ship ex: generator messages on a device.

Usage

To create a new translatable string (all assemblies):

• Open Java.Interop.Localization.Resources.resx in an IDE.
• Enter the string lookup key, string value, and any notes for translators.
• The string can now be retrieved via Java.Interop.Localization.Resources.MyStringKey.

To use the string in generator.exe:

• Create a new static member in Utilities/Report.cs that assigns a build code to the string.
  • ex: public static LocalizedMessage WarningUnknownReturnType => new LocalizedMessage (8700, Java.Interop.Localization.Resources.Generator_BG8700);
• Use Report.LogCodedWarning or Report.LogCodedError to create an MSBuild formatted build warning/error.
  • ex: Report.LogCodedError (Report.ErrorFailedToProcessEnumMap);
  • Overloads are available that take exceptions, file/line/col info, and string.Format parameters.

[brendanzagaeski]

Thanks a bunch for all this work! This looks pretty good already.

I'll leave this first set of comments to start, in particular to make sure I share the question about the argument order of LogCodedEror().

I'll plan to keep looking through the individual messages a bit more tomorrow for any suggestions to help the translation team.

[brendanzagaeski]

This version of the NuGet package looks good since it matches https://github.com/dotnet/arcade/blob/d7e47e893c86c3c9f4e8d8129b2d45516a69bc41/eng/Version.Details.xml#L59

(In case any other PR reviewers might wonder, the version in xamarin-android can be updated too. That version hasn't yet been updated to resynchronize with dotnet/arcade since back in the original commit.)

[brendanzagaeski]

The convention for generator.exe will be to set up an additional, more friendly, property name like ErrorMissingAttrName for each error message, and it will be that friendly property name that's used in the call to LogCodedError() or LogCodedWarning()?

The reason I ask is it means it takes two rounds of "find all references" to get to the LogCoded*() invocation if starting from the property Generator_BG4307 in Visual Studio. That's not a problem really, but it was a topic that came up in xamarin-android, so I just wanted to make sure I'm picturing it all correctly. (In contrast, in this current PR, doing a grep for the message code will turn up the LogCoded*() invocation in one step, as long as the person searching knows the convention that the message code is listed in a comment there.)

[jpobst]

I don't have much of an opinion here. I felt this approach mimicked the existing approach in generator. I think it's more about finding where an error/warning message code comes from rather than the text.

For example, say someone reported a BG4306 error and you want to find where in the code it comes from. There is no Resources.Generator_BG4306 because we don't want to duplicate strings in resources.resx because that causes extra translator work.

public static LocalizedMessage ErrorMoveNodeInvalidXPath => new LocalizedMessage (4305, Java.Interop.Localization.Resources.Generator_BG4300);
public static LocalizedMessage ErrorRemoveAttrInvalidXPath => new LocalizedMessage (4306, Java.Interop.Localization.Resources.Generator_BG4300);

Searching for BG4306 would technically work because of the comment, but most LogCodedError () invocations don't have a comment. So for other cases, you could at least you can scroll through Report.cs and quickly find the error message.

So it feels like the benefit of this is:

• It ties message codes to message strings, since they aren't a 1:1 mapping.
• Provides a single place where you can see all codes that can be logged.

But still, I'm not very attached to this approach. Any approach feels like a pretty minor detail that will rarely be interacted with. 🤷

[jpobst]

Maybe the "friendly name" is an annoying additional thing to name, and we should just call them the message code to make it easier to find:

public static LocalizedMessage BG4305 => new LocalizedMessage (4305, Java.Interop.Localization.Resources.Generator_BG4300);

instead of

public static LocalizedMessage ErrorMoveNodeInvalidXPath => new LocalizedMessage (4305, Java.Interop.Localization.Resources.Generator_BG4300);

The friendly naming was another case of "following the already established approach".

[brendanzagaeski]

Ooh, yeah, that approach you mentioned for cases where the same string is used for two error codes is a nice touch. In the one case where an identical string was used by two different codes in xamarin-android, I just did the naive thing and made two copies of the string under separate resource names. I believe the translation team has a process where they check new strings against a dictionary of existing translations so that they avoid duplicating translation work, but all the nicer to take an approach in the .resx file that avoids that question entirely.

On naming, the current descriptive names work fine for me, but I'd also be a-OK with having the error codes be the names, so whichever one you like best will be good.

[brendanzagaeski]

As a note to any other PR reviewers who might be skimming this PR outside of an IDE and curious about it, this is calling the following overload:

void LogCodedError (LocalizedMessage message, Exception innerException, XNode node, params object[] args)

This overload is specific to Java.Interop. In xamarin-android, the only LogCodedError() overloads that take non-string arguments always take a file name and line number.

It might just be me, but one thing that trips me up with this Java.Interop overload, even in an IDE, is that the message string is combined together with the error code in the first argument, separating it from the params array. The LogCodedError() overloads in xamarin-android and the one LogError() overload in Microsoft.Build.Utilities put the error code first, then other arguments, and then finally the message string as the second-to-last argument before the params array.

The original Resport.Error() method had the same arrangement too, with the message string as the second-to-last argument.

I find it trickier to line up the params array items with the format item numbers in this new Java.Interop overload.

[brendanzagaeski]

After stepping away and coming back to this, I had the thought that one way to put the message string second-to-last could be to change the overload signature to:

void LogCodedError (int code, Exception innerException, XNode node, string value, params object[] args)

And call it like:

LogCodedError (Report.ErrorRemoveNodeInvalidXPath.Code, e, metaitem, Report.ErrorRemoveNodeInvalidXPath.Value, path);

Admittedly, that makes the call less succinct, and it loses the type checking and IntelliSense advantages of using LocalizedMessage as the argument type. Those might have been exactly the motivations for the LocalizedMessage class in the first place, so no worries if your preference is to keep the current approach for those reasons.

[brendanzagaeski]

Thanks a bunch for the little adjustments to the .resx file already. I've gone through the rest of the resource strings now, so I'll leave this set of comments, which I think will be the last main batch from me.

I'll plan to take one more sanity check over the full PR tomorrow or after the next set of adjustments. Thanks again!

[brendanzagaeski]

No change needed, but just to highlight it, this is one more, slightly different overload variation of LogCoded*(). In this case, although the first argument is an int, that int is not the error code. I suspect this might be the best compromise available for the moment because the original Warning() and Error() methods had this difference too, so it'll just be a small tricky thing to keep in mind for now.

[jpobst]

I changed Log*:

public static void LogCodedError (LocalizedMessage message, Exception innerException, params object[] args)
to
public static void LogCodedError (LocalizedMessage message, Exception innerException, params string[] args)

While it doesn't change readability, it does ensure that things intended for string.Format do not end up as other parameters, and vice-versa.

[brendanzagaeski]

Thanks once more. This looks good! I'll leave comments for a couple last tiny optional adjustments, but otherwise I think this is pretty much all set.

As a small verification, I checked that the localizations propagated as desired into xabuild builds (by setting CultureInfo.DefaultThreadCurrentCulture and CultureInfo.DefaultThreadCurrentUICulture at the top of Xamarin.Android.Binder.CodeGenerator.Main()). The strings from the .xlf files appeared in the warnings for a test bindings library as desired 👍

I noticed two remaining Console.Error.WriteLines() strings that could in principle also be moved to the .resx file:

https://github.com/xamarin/java.interop/blob/007b35b489173010de038c95cb2a0d3ec514f30d/tools/generator/CodeGeneratorOptions.cs#L163-L166

But I'd say those are optional for now because they wouldn't appear when calling generator via the MSBuild targets. It might make sense to leave those for a separate pass, if for example, there ends up being a future pass to make exception messages localizable too.

I changed Log*...

Nice touch. I like it.

[jpobst]

Thanks for the help and review!