Fix localization irregularities. #553

BillyONeal · 2022-05-22T23:05:28Z

It fixes all violations of localization guidelines:

Repeating "error:", "Error:", "warning:", or "Warning:" at the start of a message in localized blocks.
Indenting in localized blocks.
Ending localized blocks with newlines.

and adds enforcement to make sure we don't regress these.

I believe appendnl hails from a time when we thought we were going to be much more rigid about manipulation of LocalizedStrings as tokens. There are localized strings which themselves contain newlines. There are also formatted data strings, such as error message blocks join'd with \n, that we need to insert into localized messages. We also aren't likely to ever want to change our newlines to \r\n or something like that :) To that end, I think both original reasons for this to exist are gone, and it should go away. I think we should go all the way to removal rather than only removing the guidelines about it, because I think we, and contributors, will be confused if it exists and is relatively meaningless in the same way that people are confused (and use) std::endl today. The fact is that newlines aren't special in our LocalizedString structures, and we should be honest about that in our design. Contrast/note: append_indent must stay, and we must add a plain std::string version of that, to ensure consistency in how we handle indenting, as this is likely to be actually ambiguous (as opposed to newlines which are always and forever \n in vcpkg).

…enerates comments.

… println or similar components that already insert newlines.

…b rather than email.

…ocalized messages, and fix remaining edge case occurrences. Also removes allow-incorrect-comments from generate-message-map considering that we control all inputs here and extending this to block these other lints seemed unnecessary.

# Conflicts: # include/vcpkg/base/messages.h # src/vcpkg/base/parse.cpp # src/vcpkg/build.cpp

BillyONeal · 2022-05-23T18:59:02Z

src/vcpkg/base/messages.cpp

@@ -250,6 +250,28 @@ namespace vcpkg::msg

    ::size_t detail::number_of_messages() { return messages().names.size(); }

+    std::string detail::format_examples_for_args(StringView extra_comment,


This whole thing is just to fix trailing whitespace in the comments of localization messages. It was sucked out into a .cpp (after being flattened from template args to a struct in the header) so that it could use Strings::join without creating header circularity or making messages.h (which is included in everything now) huge.

BillyONeal · 2022-05-23T19:00:27Z

src/vcpkg/commands.generate-message-map.cpp

        LocalizedString format_string_parsing_error;
        Json::Object obj;
        for (Message& msg : messages)
        {
+            if (msg.name != "ErrorMessage" && Strings::case_insensitive_ascii_starts_with(msg.value, "error:"))


These are the new lints. Note that I removed the switch to turn the lints off; we control all these inputs.

BillyONeal · 2022-05-23T19:00:45Z

src/vcpkg/install.cpp

@@ -860,36 +860,30 @@ namespace vcpkg::Install
        return ret;
    }

-    DECLARE_AND_REGISTER_MESSAGE(ErrorRequirePackagesToInstall,


This message was deleted as it is unreferenced.

BillyONeal · 2022-05-23T19:02:05Z

src/vcpkg/packagespec.cpp

@@ -76,7 +73,7 @@ namespace vcpkg
    {
        if (platform)
        {
-            return {msg::format(msgIllegalPlatformSpec).data(), expected_right_tag};
+            return {msg::format(msg::msgErrorMessage).append(msgIllegalPlatformSpec).data(), expected_right_tag};


I don't want to mess with it in this change, but I think we need a shorter way to get to msg::format(msg::msgErrorMessage)

format_error() has my vote.

BillyONeal · 2022-05-23T19:02:54Z

src/vcpkg/portfileprovider.cpp

+        VersionSpecMismatch,
+        (msg::path, msg::expected_version, msg::actual_version),
+        "",
+        "Failed to load port because versions are inconsistent. The file \"{path}\" contains the version "


I rewrote this error to make sense without the indent formatting. I could resolve it differently by breaking it up in to separate messages to preserve the indenting if people want.

include/vcpkg/base/messages.h

src/vcpkg/commands.generate-message-map.cpp

ras0219-msft · 2022-05-23T20:35:21Z

src/vcpkg/packagespec.cpp

@@ -76,7 +73,7 @@ namespace vcpkg
    {
        if (platform)
        {
-            return {msg::format(msgIllegalPlatformSpec).data(), expected_right_tag};
+            return {msg::format(msg::msgErrorMessage).append(msgIllegalPlatformSpec).data(), expected_right_tag};


format_error() has my vote.

ras0219-msft · 2022-05-23T20:38:27Z

src/vcpkg/vcpkgpaths.cpp

@@ -597,10 +597,10 @@ namespace vcpkg

    DECLARE_AND_REGISTER_MESSAGE(
        ErrorMissingVcpkgRoot,
-        (msg::url),
+        (),


I had extracted out the url since it isn't localizable content.

I think it's OK to for messages to have some content which isn't localizable; for example we talk about command line flags and stuff that aren't localized either. The comment needs to describe this. (Putting it back though)

Actually, not putting it back because I changed these instructions to ask people to file GitHub issues.

…ning, respectively.

# Conflicts: # locales/messages.json # src/vcpkg/registries.cpp

ras0219-msft · 2022-05-24T22:50:32Z

src/vcpkg/build.cpp

@@ -268,7 +268,7 @@ namespace vcpkg::Build
            LocalizedString errorMsg = msg::format(msg::msgErrorMessage).append(msgBuildDependenciesMissing);
            for (const auto& p : result.unmet_dependencies)
            {
-                errorMsg.append_indent().append_raw(p.to_string()).append_raw('\n');
+                errorMsg.append_raw('\n').append_indent().append_raw(p.to_string());


Noting that this changes behavior (one newline instead of two)

ras0219-msft · 2022-05-24T22:55:15Z

src/vcpkg/build.cpp

-        "architecture was {arch}\n    "
-        "The selected Visual Studio instance is at {path}\n    The available toolchain combinations are {list}\n");
+        "in triplet {triplet}: Unable to find a valid toolchain for requested target architecture {arch}.\n"
+        "The selected Visual Studio instance is at: {path}\n"


Noting that this change drops indentation.

BillyONeal · 2022-05-24T23:02:26Z

Those behavior changes seem reasonable to me.

BillyONeal added 12 commits May 20, 2022 18:42

Improve formatting of generated comments, and deduplicate code that g…

415aa51

…enerates comments.

Remove extra newlines from localized messages that are being fed into…

3f58c7a

… println or similar components that already insert newlines.

Fix indents in "VcpkgHasCrashed" message. Also direct people to GitHu…

011ed7d

…b rather than email.

Also fix UnsupportedToolchain message.

6ba1110

Also fix VersionSpecMismatch message.

c53febf

Deduplicate all Error:s in localized messages.

2550481

Also block trailing newlines.

56d2aa7

Merge remote-tracking branch 'origin/main' into fix-localized-newlines

656dbd0

# Conflicts: # include/vcpkg/base/messages.h # src/vcpkg/base/parse.cpp # src/vcpkg/build.cpp

Fix end to end test.

d443b74

Merge remote-tracking branch 'origin/main' into fix-localized-newlines

d4f9b34

BillyONeal commented May 23, 2022

View reviewed changes

ras0219-msft reviewed May 23, 2022

View reviewed changes

BillyONeal added 2 commits May 23, 2022 14:05

Rename print_error and print_warning to println_error and println_war…

3551070

…ning, respectively.

Merge remote-tracking branch 'origin/main' into fix-localized-newlines

b2a3714

# Conflicts: # locales/messages.json # src/vcpkg/registries.cpp

ras0219-msft mentioned this pull request May 23, 2022

Localize edit command and binary caching #551

Merged

Thomas1664 mentioned this pull request May 24, 2022

[build] Optimizations and Localization #555

Merged

ras0219-msft approved these changes May 24, 2022

View reviewed changes

BillyONeal merged commit 4ba70e9 into microsoft:main May 24, 2022

BillyONeal deleted the fix-localized-newlines branch May 24, 2022 23:02

BillyONeal mentioned this pull request Jun 28, 2022

Localization of strings in binarycaching.cpp #606

Merged

BillyONeal mentioned this pull request Sep 9, 2022

Add new post check for share folder #675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix localization irregularities. #553

Fix localization irregularities. #553

BillyONeal commented May 22, 2022 •

edited

Loading

BillyONeal May 23, 2022

BillyONeal May 23, 2022

BillyONeal May 23, 2022

BillyONeal May 23, 2022

ras0219-msft May 23, 2022

BillyONeal May 23, 2022

ras0219-msft May 23, 2022

ras0219-msft May 23, 2022

BillyONeal May 23, 2022

BillyONeal May 23, 2022

ras0219-msft May 24, 2022

ras0219-msft May 24, 2022

BillyONeal commented May 24, 2022

		@@ -250,6 +250,28 @@ namespace vcpkg::msg

		::size_t detail::number_of_messages() { return messages().names.size(); }

		std::string detail::format_examples_for_args(StringView extra_comment,

Fix localization irregularities. #553

Fix localization irregularities. #553

Conversation

BillyONeal commented May 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented May 24, 2022

BillyONeal commented May 22, 2022 •

edited

Loading