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

[Localization] Monoceros #704

Merged
merged 13 commits into from
Oct 4, 2022

Conversation

JavierMatosD
Copy link
Contributor

@JavierMatosD JavierMatosD commented Sep 14, 2022

Localizes messages in the following files:

  • statusparagraph.cpp
  • statusparagraphs.cpp
  • remove.cpp
  • portfileprovider.cpp

@JavierMatosD JavierMatosD marked this pull request as ready for review September 16, 2022 19:27
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust you to make these nitpick fixes. Do note the request to implement "MutuallyExclusiveOption" back in Aries

include/vcpkg/base/messages.h Outdated Show resolved Hide resolved
@@ -52,7 +51,7 @@ namespace vcpkg::Remove
const auto status = fs.symlink_status(target, ec);
if (ec)
{
print2(Color::error, "failed: symlink_status(", target, "): ", ec.message(), "\n");
msg::println_error(msgFailedSymlinkStatus, msg::path = target, msg::error_msg = ec.message());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the same formatting mechanism as exit_filesystem_call_error (and FailedSymlinkStatus deleted)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto other filesystem operations here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In files.cpp there's a function exit_system_call_error which forms an error message out of the name of the called operation, the parameters, and the actual error returned by the operating system, which notably means no localization is necessary. (It is done by the operating system) This comment is asking to extract that and print that instead rather than making slightly different localized message for each "generic filesystem operation failed" message.

auto arguments = args.size() == 0 ? "()" : "(\"" + Strings::join("\", \"", args.begin(), args.end()) + "\")";
Checks::exit_with_message_and_line(li, Strings::concat(call_name, arguments, ": ", ec.message()));

src/vcpkg/statusparagraphs.cpp Outdated Show resolved Hide resolved
include/vcpkg/base/messages.h Outdated Show resolved Hide resolved
include/vcpkg/base/messages.h Outdated Show resolved Hide resolved
src/vcpkg/portfileprovider.cpp Show resolved Hide resolved
locales/messages.en.json Outdated Show resolved Hide resolved
* improve message clarity
* add explanation for InvalidOptionForRemove to avoid confusion amongst translators
* add overload for Checks::unreachable that takes a message
* add function format_filesystem_call_error
# Conflicts:
#	include/vcpkg/base/messages.h
#	locales/messages.en.json
#	locales/messages.json
#	src/vcpkg/base/messages.cpp
Comment on lines 97 to 102
fs.remove(*b, ec);
if (ec)
{
msg::write_unlocalized_text_to_stdout(Color::error, ec.message());
Checks::exit_with_message_and_line(VCPKG_LINE_INFO,
format_filesystem_call_error(ec, __func__, {*b}));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

                    fs.remove(*b, ec);
                    if (ec)
                    {
                        Checks::exit_with_message_and_line(VCPKG_LINE_INFO,
                                                           format_filesystem_call_error(ec, __func__, {*b}));
                    }

should instead be

                    fs.remove(*b, VCPKG_LINE_INFO);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops looks like the red code didn't terminate here so it should do that.

@@ -19,6 +19,15 @@ namespace vcpkg::Remove

REGISTER_MESSAGE(RemovingPackage);

static LocalizedString format_filesystem_call_error(const std::error_code& ec,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This belongs in files.cpp and exit_filesystem_call_error should use it too.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with 1 nitpick

@JavierMatosD JavierMatosD merged commit 6b0fb0e into microsoft:main Oct 4, 2022
@JavierMatosD JavierMatosD deleted the Localization_Monoceros branch April 20, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants