From 9240c94e5926bd6b3e7ab234edff1f9713d8a4af Mon Sep 17 00:00:00 2001 From: Ygg01 Date: Wed, 5 Jun 2024 07:23:41 +0200 Subject: [PATCH] Write Errors when a duplicate localization key is found. (#4885) * Update Linguini to v0.8.1 * Add tests and verify desired behavior has been reached. * Remove duplicate messages. * Minor fix to message output. Add Wrapper for Fluent errors. * Restart the test pipeline. * Restart the test pipeline. * Make so test don't do an early bailout. * Ensure all errors get written rather than bailing on first. * Fix text breakage. * Remove obsolete // TODO LINGUINI * line wrapping conventions --------- Co-authored-by: metalgearsloth Co-authored-by: ElectroJr --- Directory.Packages.props | 8 +- Resources/Locale/en-US/commands.ftl | 21 +-- Robust.Shared/Localization/LocHelper.cs | 47 ++++++ .../LocalizationManager.Entity.cs | 5 +- .../LocalizationManager.Functions.cs | 38 ++++- .../Localization/LocalizationManager.cs | 129 +++++++++++----- .../Localization/LoadLocalizationTest.cs | 43 ++++++ Robust.UnitTesting/SpyLogManager.cs | 146 ++++++++++++++++++ 8 files changed, 368 insertions(+), 69 deletions(-) create mode 100644 Robust.UnitTesting/Shared/Localization/LoadLocalizationTest.cs create mode 100644 Robust.UnitTesting/SpyLogManager.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 24284c3e6..98cdd7ae0 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -15,10 +15,10 @@ - + - - + + @@ -71,4 +71,4 @@ - + \ No newline at end of file diff --git a/Resources/Locale/en-US/commands.ftl b/Resources/Locale/en-US/commands.ftl index 22b8f164a..5caef6794 100644 --- a/Resources/Locale/en-US/commands.ftl +++ b/Resources/Locale/en-US/commands.ftl @@ -252,9 +252,6 @@ cmd-bind-arg-command = cmd-net-draw-interp-desc = Toggles the debug drawing of the network interpolation. cmd-net-draw-interp-help = Usage: net_draw_interp -cmd-net-draw-interp-desc = Toggles the debug drawing of the network interpolation. -cmd-net-draw-interp-help = Usage: net_draw_interp - cmd-net-watch-ent-desc = Dumps all network updates for an EntityId to the console. cmd-net-watch-ent-help = Usage: net_watchent <0|EntityUid> @@ -306,16 +303,9 @@ cmd-savegrid-help = savegrid cmd-testbed-desc = Loads a physics testbed on the specified map. cmd-testbed-help = testbed -cmd-saveconfig-desc = Saves the client configuration to the config file. -cmd-saveconfig-help = saveconfig - ## 'flushcookies' command # Note: the flushcookies command is from Robust.Client.WebView, it's not in the main engine code. -cmd-flushcookies-desc = Flush CEF cookie storage to disk -cmd-flushcookies-help = This ensure cookies are properly saved to disk in the event of unclean shutdowns. - Note that the actual operation is asynchronous. - ## 'addcomp' command cmd-addcomp-desc = Adds a component to an entity. cmd-addcomp-help = addcomp @@ -453,9 +443,6 @@ cmd-showanchored-help = Usage: showanchored cmd-dmetamem-desc = Dumps a type's members in a format suitable for the sandbox configuration file. cmd-dmetamem-help = Usage: dmetamem -cmd-dmetamem-desc = Displays chunk bounds for the purposes of rendering. -cmd-dmetamem-help = Usage: showchunkbb - cmd-launchauth-desc = Load authentication tokens from launcher data to aid in testing of live servers. cmd-launchauth-help = Usage: launchauth @@ -522,9 +509,6 @@ cmd-profsnap-help = Usage: profsnap cmd-devwindow-desc = Dev Window cmd-devwindow-help = Usage: devwindow -cmd-devwindow-desc = Open file -cmd-devwindow-help = Usage: testopenfile - cmd-scene-desc = Immediately changes the UI scene/state. cmd-scene-help = Usage: scene @@ -535,14 +519,11 @@ cmd-hwid-desc = Returns the current HWID (HardWare ID). cmd-hwid-help = Usage: hwid cmd-vvread-desc = Retrieve a path's value using VV (View Variables). -cmd-vvread-desc = Usage: vvread +cmd-vvread-help = Usage: vvread cmd-vvwrite-desc = Modify a path's value using VV (View Variables). cmd-vvwrite-help = Usage: vvwrite -cmd-vv-desc = Opens View Variables (VV). -cmd-vv-help = Usage: vv - cmd-vvinvoke-desc = Invoke/Call a path with arguments using VV. cmd-vvinvoke-help = Usage: vvinvoke [arguments...] diff --git a/Robust.Shared/Localization/LocHelper.cs b/Robust.Shared/Localization/LocHelper.cs index 11ee9b8b2..5e41017e9 100644 --- a/Robust.Shared/Localization/LocHelper.cs +++ b/Robust.Shared/Localization/LocHelper.cs @@ -1,6 +1,10 @@ using System; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Text; +using Linguini.Bundle; using Linguini.Bundle.Errors; +using Linguini.Syntax.Ast; using Linguini.Syntax.Parser.Error; using Robust.Shared.Collections; using Robust.Shared.Utility; @@ -17,6 +21,24 @@ internal static class LocHelper return FormatErrors(self.Message, span, resource, newLine); } + public static bool InsertResourcesAndReport(this FluentBundle bundle, Resource resource, + ResPath path, [NotNullWhen(false)] out List? errors) + { + if (!bundle.AddResource(resource, out var parseErrors)) + { + errors = new List(); + foreach (var fluentError in parseErrors) + { + errors.Add(new LocError(path, fluentError)); + } + + return false; + } + + errors = null; + return true; + } + private static string FormatErrors(string message, ErrorSpan span, ReadOnlyMemory resource, string? newLine) { newLine ??= Environment.NewLine; @@ -69,3 +91,28 @@ internal static class LocHelper return sb.ToString(); } } +/// +/// Wrapper around Fluent Error, that adds path to the list of values. +/// Work in progress, FluentErrors need to be modified to be more accessible. +/// +internal record LocError +{ + public readonly ResPath Path; + public readonly FluentError Error; + + /// + /// Basic constructor. + /// + /// path of resource being added. + /// FluentError encountered. + public LocError(ResPath path, FluentError fluentError) + { + Path = path; + Error = fluentError; + } + + public override string ToString() + { + return $"[{Path.CanonPath}]: {Error}"; + } +} diff --git a/Robust.Shared/Localization/LocalizationManager.Entity.cs b/Robust.Shared/Localization/LocalizationManager.Entity.cs index 7ae177f7e..c3daf4931 100644 --- a/Robust.Shared/Localization/LocalizationManager.Entity.cs +++ b/Robust.Shared/Localization/LocalizationManager.Entity.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics.CodeAnalysis; using System.Linq; +using Linguini.Bundle; using Linguini.Bundle.Errors; using Robust.Shared.GameObjects; using Robust.Shared.GameObjects.Components.Localization; @@ -92,14 +93,14 @@ namespace Robust.Shared.Localization var allErrors = new List(); if (desc == null - && !bundle.TryGetMsg(locId, "desc", null, out var err1, out desc)) + && !bundle.TryGetMessage(locId, "desc", null, out var err1, out desc)) { desc = null; allErrors.AddRange(err1); } if (suffix == null - && !bundle.TryGetMsg(locId, "suffix", null, out var err, out suffix)) + && !bundle.TryGetMessage(locId, "suffix", null, out var err, out suffix)) { suffix = null; } diff --git a/Robust.Shared/Localization/LocalizationManager.Functions.cs b/Robust.Shared/Localization/LocalizationManager.Functions.cs index db192ee06..356460a3e 100644 --- a/Robust.Shared/Localization/LocalizationManager.Functions.cs +++ b/Robust.Shared/Localization/LocalizationManager.Functions.cs @@ -1,16 +1,13 @@ -#nullable enable -using System; +using System; using System.Collections.Generic; using System.Globalization; using System.Linq; using System.Text.RegularExpressions; using Linguini.Bundle; -using Linguini.Bundle.Types; using Linguini.Shared.Types.Bundle; using Robust.Shared.Enums; using Robust.Shared.GameObjects; using Robust.Shared.GameObjects.Components.Localization; -using Robust.Shared.IoC; using Robust.Shared.Maths; namespace Robust.Shared.Localization @@ -326,8 +323,7 @@ namespace Robust.Shared.Localization private void AddCtxFunction(FluentBundle ctx, string name, LocFunction function) { - ctx.AddFunction(name, (args, options) - => CallFunction(function, ctx, args, options), out _, InsertBehavior.Overriding); + ctx.AddFunctionOverriding(name, (args, options) => CallFunction(function, ctx, args, options)); } private IFluentType CallFunction( @@ -356,8 +352,8 @@ namespace Robust.Shared.Localization { var bundle = _contexts[culture]; - bundle.AddFunction(name, (args, options) - => CallFunction(function, bundle, args, options), out _, InsertBehavior.Overriding); + bundle.AddFunctionOverriding(name, (args, options) + => CallFunction(function, bundle, args, options)); } } @@ -377,6 +373,32 @@ namespace Robust.Shared.Localization return WrappedValue.Format(_context); } + public bool IsError() + { + return false; + } + + public bool Matches(IFluentType other, IScope scope) + { + if (other is FluentLocWrapperType otherWrapper) + { + return (WrappedValue, otherWrapper.WrappedValue) switch + { + (LocValueNone, LocValueNone) => true, + (LocValueDateTime l, LocValueDateTime d) => l.Value.Equals(d.Value), + (LocValueTimeSpan l, LocValueTimeSpan d) => l.Value.Equals(d.Value), + (LocValueNumber l, LocValueNumber d) => l.Value.Equals(d.Value), + (LocValueString l, LocValueString d) => l.Value.Equals(d.Value), + (LocValueEntity l, LocValueEntity d) => l.Value.Equals(d.Value), + ({ } l, { } d) => Equals(l, d), + _ => false, + }; + } + + return false; + + } + public IFluentType Copy() { return this; diff --git a/Robust.Shared/Localization/LocalizationManager.cs b/Robust.Shared/Localization/LocalizationManager.cs index bf0cad21a..8fdbeaf01 100644 --- a/Robust.Shared/Localization/LocalizationManager.cs +++ b/Robust.Shared/Localization/LocalizationManager.cs @@ -4,6 +4,7 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; using System.Linq; +using System.Text; using Linguini.Bundle; using Linguini.Bundle.Builder; using Linguini.Bundle.Errors; @@ -47,7 +48,8 @@ namespace Robust.Shared.Localization if (!TryGetString(messageId, out var msg)) { - _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, messageId); + _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, + messageId); msg = messageId; } @@ -64,8 +66,9 @@ namespace Robust.Shared.Localization if (TryGetString(messageId, out var argMsg, arg)) return argMsg; - _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, messageId); - return messageId; + _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, + messageId); + return messageId; } public string GetString(string messageId, (string, object) arg1, (string, object) arg2) @@ -76,8 +79,9 @@ namespace Robust.Shared.Localization if (TryGetString(messageId, out var argMsg, arg1, arg2)) return argMsg; - _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, messageId); - return messageId; + _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, + messageId); + return messageId; } public string GetString(string messageId, params (string, object)[] args) @@ -88,8 +92,9 @@ namespace Robust.Shared.Localization if (TryGetString(messageId, out var argMsg, args)) return argMsg; - _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, messageId); - return messageId; + _logSawmill.Debug("Unknown messageId ({culture}): {messageId}", _defaultCulture.Value.Item1.Name, + messageId); + return messageId; } #endregion @@ -101,7 +106,7 @@ namespace Robust.Shared.Localization #region TryGetString -public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value) + public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value) { if (_defaultCulture == null) { @@ -112,7 +117,7 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value if (TryGetString(messageId, _defaultCulture.Value, out value)) return true; - foreach (var fallback in _fallbackCultures) + foreach (var fallback in _fallbackCultures) { if (TryGetString(messageId, fallback, out value)) return true; @@ -122,18 +127,24 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value return false; } - public bool TryGetString(string messageId, (CultureInfo, FluentBundle) bundle, [NotNullWhen(true)] out string? value) + public bool TryGetString(string messageId, + (CultureInfo, FluentBundle) bundle, + [NotNullWhen(true)] out string? value) { try { - // TODO LINGUINI error list nullable. - var result = bundle.Item2.TryGetAttrMsg(messageId, null, out var errs, out value); - foreach (var err in errs) + if (bundle.Item2.TryGetAttrMessage(messageId, null, out var errors, out value)) + return true; + + if (errors != null) { - _logSawmill.Error("{culture}/{messageId}: {error}", bundle.Item1.Name, messageId, err); + foreach (var err in errors) + { + _logSawmill.Error("{culture}/{messageId}: {error}", bundle.Item1.Name, messageId, err); + } } - return result; + return false; } catch (Exception e) { @@ -143,7 +154,8 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value } } - public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value, + public bool TryGetString(string messageId, + [NotNullWhen(true)] out string? value, (string, object) arg) { // TODO LINGUINI add try-get-message variant that takes in a (string, object)[] @@ -165,8 +177,10 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value return TryGetString(messageId, out value, args, bundle, info); } - public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value, - (string, object) arg1, (string, object) arg2) + public bool TryGetString(string messageId, + [NotNullWhen(true)] out string? value, + (string, object) arg1, + (string, object) arg2) { // TODO LINGUINI add try-get-message variant that takes in a (string, object)[] // I.e., have it automatically call FluentFromObject(context) with the right context if the message exists @@ -188,7 +202,8 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value return TryGetString(messageId, out value, args, bundle, info); } - public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value, + public bool TryGetString(string messageId, + [NotNullWhen(true)] out string? value, params (string, object)[] keyArgs) { // TODO LINGUINI add try-get-message variant that takes in a (string, object)[] @@ -216,10 +231,13 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value { try { - var result = bundle.TryGetAttrMsg(messageId, args, out var errs, out value); - foreach (var err in errs) + var result = bundle.TryGetAttrMessage(messageId, args, out var errs, out value); + if (errs != null) { - _logSawmill.Error("{culture}/{messageId}: {error}", culture.Name, messageId, err); + foreach (var err in errs) + { + _logSawmill.Error("{culture}/{messageId}: {error}", culture.Name, messageId, err); + } } return result; @@ -245,14 +263,14 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value } var idx = messageId.IndexOf('.'); - if (idx != -1 ) + if (idx != -1) messageId = messageId.Remove(idx); culture = _defaultCulture; if (culture.Value.Item2.HasMessage(messageId)) return true; - foreach (var fallback in _fallbackCultures) + foreach (var fallback in _fallbackCultures) { culture = fallback; if (culture.Value.Item2.HasMessage(messageId)) @@ -279,7 +297,7 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value if (bundle.TryGetAstMessage(messageId, out message)) return true; - foreach (var fallback in _fallbackCultures) + foreach (var fallback in _fallbackCultures) { bundle = fallback.Item2; if (bundle.TryGetAstMessage(messageId, out message)) @@ -331,13 +349,13 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value _contexts.Add(culture, bundle); AddBuiltInFunctions(bundle); - _loadData(_res, culture, bundle); + _initData(_res, culture, bundle); DefaultCulture ??= culture; } public void SetFallbackCluture(params CultureInfo[] cultures) { - _fallbackCultures = Array.Empty<(CultureInfo, FluentBundle)>();; + _fallbackCultures = Array.Empty<(CultureInfo, FluentBundle)>(); var tuples = new (CultureInfo, FluentBundle)[cultures.Length]; var i = 0; foreach (var culture in cultures) @@ -376,6 +394,41 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value } private void _loadData(IResourceManager resourceManager, CultureInfo culture, FluentBundle context) + { + var resources = ReadLocaleFolder(resourceManager, culture); + + foreach (var (path, resource, data) in resources) + { + var errors = resource.Errors; + context.AddResourceOverriding(resource); + WriteWarningForErrs(path, errors, data); + } + } + + private void _initData(IResourceManager resourceManager, CultureInfo culture, FluentBundle context) + { + var resources = ReadLocaleFolder(resourceManager, culture); + + var resErrors = new List(); + foreach (var (path, resource, data) in resources) + { + var errors = resource.Errors; + WriteWarningForErrs(path, errors, data); + if (!context.InsertResourcesAndReport(resource, path, out var errs)) + { + resErrors.AddRange(errs); + } + + } + + if (resErrors.Count > 0) + { + WriteLocErrors(resErrors); + } + } + + private static ParallelQuery<(ResPath path, Resource resource, string contents)> ReadLocaleFolder( + IResourceManager resourceManager, CultureInfo culture) { // Load data from .ftl files. // Data is loaded from /Locale//* @@ -400,13 +453,7 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value var resource = parser.Parse(); return (path, resource, contents); }); - - foreach (var (path, resource, data) in resources) - { - var errors = resource.Errors; - context.AddResourceOverriding(resource); - WriteWarningForErrs(path, errors, data); - } + return resources; } private void WriteWarningForErrs(ResPath path, List errs, string resource) @@ -417,12 +464,24 @@ public bool TryGetString(string messageId, [NotNullWhen(true)] out string? value } } - private void WriteWarningForErrs(IList errs, string locId) + private void WriteWarningForErrs(IList? errs, string locId) { + if (errs == null) return; foreach (var err in errs) { _logSawmill.Error("Error extracting `{locId}`\n{e1}", locId, err); } } + + private void WriteLocErrors(IList? errs) + { + if (errs == null) return; + var sbErr = new StringBuilder(); + foreach (var err in errs) + { + sbErr.Append(err).AppendLine(); + } + _logSawmill.Error(sbErr.ToString()); + } } } diff --git a/Robust.UnitTesting/Shared/Localization/LoadLocalizationTest.cs b/Robust.UnitTesting/Shared/Localization/LoadLocalizationTest.cs new file mode 100644 index 000000000..03e079e90 --- /dev/null +++ b/Robust.UnitTesting/Shared/Localization/LoadLocalizationTest.cs @@ -0,0 +1,43 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using NUnit.Framework; +using Robust.Shared.ContentPack; +using Robust.Shared.IoC; +using Robust.Shared.Localization; +using Robust.Shared.Log; + +namespace Robust.UnitTesting.Shared.Localization; + +[TestFixture] +public sealed class LoadLocalizationTest : RobustUnitTest +{ + private const string DuplicateTerm = @" +term1 = 1 +term1 = 2 +"; + protected override void OverrideIoC() + { + base.OverrideIoC(); + + IoCManager.Register(overwrite: true); + } + + + [Test] + public void TestLoadLocalization() + { + var res = IoCManager.Resolve(); + res.MountString("/Locale/en-US/a.ftl", DuplicateTerm); + + var loc = IoCManager.Resolve(); + + var spyLog = (SpyLogManager) IoCManager.Resolve(); + var culture = new CultureInfo("en-US", false); + + var x = spyLog.CountError; + loc.LoadCulture(culture); + Assert.That(spyLog.CountError, NUnit.Framework.Is.GreaterThan(x)); + } +} + diff --git a/Robust.UnitTesting/SpyLogManager.cs b/Robust.UnitTesting/SpyLogManager.cs new file mode 100644 index 000000000..0ec00e4df --- /dev/null +++ b/Robust.UnitTesting/SpyLogManager.cs @@ -0,0 +1,146 @@ +using System; +using System.Collections.Generic; +using Robust.Shared.Log; + +namespace Robust.UnitTesting; + +/// +/// Represents a log manager that spies on logged messages. Use this to check if side effect +/// like logging is being correctly done. +/// +public sealed class SpyLogManager : ILogManager +{ + private readonly SpyLogger _spyLogger = new(); + public ISawmill RootSawmill => _spyLogger; + public ISawmill GetSawmill(string name) + { + return _spyLogger; + } + + public IEnumerable AllSawmills => new[] + { + _spyLogger + }; + + public int CountError => _spyLogger.ErrorMessages.Count; + + public void Clear() + { + _spyLogger.Clear(); + } +} + +/// +/// Represents a logger used for spying on log messages. +/// +public sealed class SpyLogger : ISawmill +{ + public string Name { get; } = "SpyLogger"; + public LogLevel? Level { get; set; } = LogLevel.Debug; + + public List DebugMessages = new(); + public List ErrorMessages = new(); + public List WarningMessages = new(); + public List InfoMessages = new(); + public List FatalMessages = new(); + public List VerboseMessages = new(); + + public void AddHandler(ILogHandler handler) + { + // NOT NEEDED + } + + public void RemoveHandler(ILogHandler handler) + { + // NOT NEEDED + } + + public void Log(LogLevel level, string message, params object?[] args) + { + Log(level, null, message, args); + } + + public void Log(LogLevel level, Exception? exception, string message, params object?[] args) + { + var msg = string.Format(message, args); + + var list = level switch + { + LogLevel.Verbose => VerboseMessages, + LogLevel.Debug => DebugMessages, + LogLevel.Info => InfoMessages, + LogLevel.Warning => WarningMessages, + LogLevel.Error => ErrorMessages, + LogLevel.Fatal => FatalMessages, + _ => VerboseMessages, + }; + + list.Add(msg); + } + + public void Log(LogLevel level, string message) + { + Log(level, null, message, []); + } + + public void Debug(string message, params object?[] args) + { + Log(LogLevel.Debug, null, message, args); + } + + public void Debug(string message) + { + Log(LogLevel.Debug, message); + } + + public void Info(string message, params object?[] args) + { + Log(LogLevel.Info, null, message, args); + } + + public void Info(string message) + { + Log(LogLevel.Debug, message); + } + + public void Warning(string message, params object?[] args) + { + Log(LogLevel.Warning, message, args); + } + + public void Warning(string message) + { + Log(LogLevel.Warning, message); + } + + public void Error(string message, params object?[] args) + { + Log(LogLevel.Error, message, args); + } + + public void Error(string message) + { + Log(LogLevel.Error, message); + } + + public void Fatal(string message, params object?[] args) + { + Log(LogLevel.Fatal, message, args); + } + + public void Fatal(string message) + { + Log(LogLevel.Fatal, message); + } + + public void Clear() + { + DebugMessages.Clear(); + ErrorMessages.Clear(); + WarningMessages.Clear(); + InfoMessages.Clear(); + FatalMessages.Clear(); + VerboseMessages.Clear(); + } +} +