Skip to content

Commit

Permalink
More localization friendly source strings and context commenting (#2454)
Browse files Browse the repository at this point in the history
  • Loading branch information
@AmelBawa-msft
AmelBawa-msft authored Dec 16, 2022
1 parent 6140e62 commit 66b851f
Show file tree
Hide file tree
Showing 40 changed files with 729 additions and 594 deletions.
33 changes: 2 additions & 31 deletions src/AppInstallerCLICore/ChannelStreams.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,6 @@

namespace AppInstaller::CLI::Execution
{
namespace details
{
// List of approved types for output, others are potentially not localized.
template <typename T>
struct IsApprovedForOutput
{
static constexpr bool value = false;
};

#define WINGET_CREATE_ISAPPROVEDFOROUTPUT_SPECIALIZATION(_t_) \
template <> \
struct IsApprovedForOutput<_t_> \
{ \
static constexpr bool value = true; \
}

// It is assumed that single char values need not be localized, as they are matched
// ordinally or they are punctuation / other.
WINGET_CREATE_ISAPPROVEDFOROUTPUT_SPECIALIZATION(char);
// Localized strings (and from an Id for one for convenience).
WINGET_CREATE_ISAPPROVEDFOROUTPUT_SPECIALIZATION(Resource::StringId);
WINGET_CREATE_ISAPPROVEDFOROUTPUT_SPECIALIZATION(Resource::LocString);
// Strings explicitly declared as localization independent.
WINGET_CREATE_ISAPPROVEDFOROUTPUT_SPECIALIZATION(Utility::LocIndView);
WINGET_CREATE_ISAPPROVEDFOROUTPUT_SPECIALIZATION(Utility::LocIndString);
// Normalized strings come from user data and should therefore already by localized
// by how they are chosen (or there is no localized version).
WINGET_CREATE_ISAPPROVEDFOROUTPUT_SPECIALIZATION(Utility::NormalizedString);
}

// The base stream for all channels.
struct BaseStream
{
Expand Down Expand Up @@ -102,7 +72,8 @@ namespace AppInstaller::CLI::Execution
// * If your string came from outside of the source code, it is best to store it in a
// Utility::NormalizedString so that it has a normalized representation. This also
// informs the output that there is no localized version to use.
// TODO: Convert the rest of the code base and uncomment to enforce localization.
// TODO: This assertion is currently only applied to placeholders in localized strings.
// Convert the rest of the code base and uncomment to enforce localization.
//static_assert(details::IsApprovedForOutput<std::decay_t<T>>::value, "This type may not be localized, see comment for more information");
if (m_enabled)
{
Expand Down
94 changes: 31 additions & 63 deletions src/AppInstallerCLICore/Command.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,7 @@ using namespace AppInstaller::Settings;

namespace AppInstaller::CLI
{
constexpr std::string_view s_Command_ArgName_SilentAndInteractive = "silent|interactive"sv;

const Utility::LocIndString CommandException::Message() const
{
if (m_replace)
{
return Utility::LocIndString{ Utility::FindAndReplaceMessageToken(m_message, m_replace.value()) };
}

// Fall back to just using the message.
return Utility::LocIndString{ m_message.get() };
}
constexpr Utility::LocIndView s_Command_ArgName_SilentAndInteractive = "silent|interactive"_liv;

Command::Command(
std::string_view name,
Expand All @@ -49,9 +38,8 @@ namespace AppInstaller::CLI

void Command::OutputIntroHeader(Execution::Reporter& reporter) const
{
reporter.Info() <<
(Runtime::IsReleaseBuild() ? Resource::String::WindowsPackageManager : Resource::String::WindowsPackageManagerPreview) << " v"_liv << Runtime::GetClientVersion() << std::endl <<
Resource::String::MainCopyrightNotice << std::endl;
auto productName = Runtime::IsReleaseBuild() ? Resource::String::WindowsPackageManager : Resource::String::WindowsPackageManagerPreview;
reporter.Info() << productName(Runtime::GetClientVersion()) << std::endl << Resource::String::MainCopyrightNotice << std::endl;
}

void Command::OutputHelp(Execution::Reporter& reporter, const CommandException* exception) const
Expand All @@ -63,29 +51,7 @@ namespace AppInstaller::CLI
// Error if given
if (exception)
{
auto error = reporter.Error();
error << exception->Message();

if (!exception->Params().empty())
{
error << " :"_liv;
bool first = true;
for (const auto& param : exception->Params())
{
if (first)
{
first = false;
}
else
{
error << ',';
}
error << " '"_liv << param << '\'';
}
}

error << std::endl <<
std::endl;
reporter.Error() << exception->Message() << std::endl << std::endl;
}

// Description
Expand Down Expand Up @@ -115,7 +81,7 @@ namespace AppInstaller::CLI
}

// Output the command preamble and command chain
infoOut << Resource::String::Usage << ": winget"_liv << Utility::LocIndView{ commandChain };
infoOut << Resource::String::Usage("winget"_liv, Utility::LocIndView{ commandChain });

auto commandAliases = Aliases();
auto commands = GetVisibleCommands();
Expand Down Expand Up @@ -280,10 +246,10 @@ namespace AppInstaller::CLI
}

// Finally, the link to the documentation pages
std::string helpLink = HelpLink();
auto helpLink = Utility::LocIndString{ HelpLink() };
if (!helpLink.empty())
{
infoOut << std::endl << Resource::String::HelpLinkPreamble << ' ' << helpLink << std::endl;
infoOut << std::endl << Resource::String::HelpLinkPreamble(helpLink) << std::endl;
}
}

Expand Down Expand Up @@ -314,7 +280,7 @@ namespace AppInstaller::CLI
{
auto feature = ExperimentalFeature::GetFeature(command->Feature());
AICLI_LOG(CLI, Error, << "Trying to use command: " << *itr << " without enabling feature " << feature.JsonName());
throw CommandException(Resource::String::FeatureDisabledMessage, feature.JsonName());
throw CommandException(Resource::String::FeatureDisabledMessage(feature.JsonName()));
}

if (!Settings::GroupPolicies().IsEnabled(command->GroupPolicy()))
Expand All @@ -331,7 +297,7 @@ namespace AppInstaller::CLI
}

// TODO: If we get to a large number of commands, do a fuzzy search much like git
throw CommandException(Resource::String::UnrecognizedCommand, *itr);
throw CommandException(Resource::String::UnrecognizedCommand(Utility::LocIndView{ *itr }));
}

// The argument parsing state machine.
Expand Down Expand Up @@ -367,14 +333,14 @@ namespace AppInstaller::CLI
const std::optional<Execution::Args::Type>& Type() const { return m_type; }

// The actual argument string associated with Type.
const std::string& Arg() const { return m_arg; }
const Utility::LocIndString& Arg() const { return m_arg; }

// If set, indicates that the last argument produced an error.
const std::optional<CommandException>& Exception() const { return m_exception; }

private:
std::optional<Execution::Args::Type> m_type;
std::string m_arg;
Utility::LocIndString m_arg;
std::optional<CommandException> m_exception;
};

Expand Down Expand Up @@ -432,7 +398,7 @@ namespace AppInstaller::CLI
// If the next argument was to be a value, but none was provided, convert it to an exception.
else if (m_state.Type() && m_invocationItr == m_invocation.end())
{
throw CommandException(Resource::String::MissingArgumentError, m_state.Arg());
throw CommandException(Resource::String::MissingArgumentError(m_state.Arg()));
}
}

Expand Down Expand Up @@ -472,7 +438,7 @@ namespace AppInstaller::CLI
// 4. If the argument is only a double --, all further arguments are only considered as positional.
ParseArgumentsStateMachine::State ParseArgumentsStateMachine::StepInternal()
{
std::string_view currArg = *m_invocationItr;
auto currArg = Utility::LocIndView{ *m_invocationItr };
++m_invocationItr;

// If the previous step indicated a value was needed, set it and forget it.
Expand All @@ -488,15 +454,15 @@ namespace AppInstaller::CLI
const CLI::Argument* nextPositional = NextPositional();
if (!nextPositional)
{
return CommandException(Resource::String::ExtraPositionalError, currArg);
return CommandException(Resource::String::ExtraPositionalError(currArg));
}

m_executionArgs.AddArg(nextPositional->ExecArgType(), currArg);
}
// The currentArg must not be empty, and starts with a -
else if (currArg.length() == 1)
{
return CommandException(Resource::String::InvalidArgumentSpecifierError, currArg);
return CommandException(Resource::String::InvalidArgumentSpecifierError(currArg));
}
// Now it must be at least 2 chars
else if (currArg[1] != APPINSTALLER_CLI_ARGUMENT_IDENTIFIER_CHAR)
Expand All @@ -507,7 +473,7 @@ namespace AppInstaller::CLI
auto itr = std::find_if(m_arguments.begin(), m_arguments.end(), [&](const Argument& arg) { return (currChar == arg.Alias()); });
if (itr == m_arguments.end())
{
return CommandException(Resource::String::InvalidAliasError, currArg);
return CommandException(Resource::String::InvalidAliasError(currArg));
}

if (itr->Type() == ArgumentType::Flag)
Expand All @@ -521,11 +487,11 @@ namespace AppInstaller::CLI
auto itr2 = std::find_if(m_arguments.begin(), m_arguments.end(), [&](const Argument& arg) { return (currChar == arg.Alias()); });
if (itr2 == m_arguments.end())
{
return CommandException(Resource::String::AdjoinedNotFoundError, currArg);
return CommandException(Resource::String::AdjoinedNotFoundError(currArg));
}
else if (itr2->Type() != ArgumentType::Flag)
{
return CommandException(Resource::String::AdjoinedNotFlagError, currArg);
return CommandException(Resource::String::AdjoinedNotFlagError(currArg));
}
else
{
Expand All @@ -541,7 +507,7 @@ namespace AppInstaller::CLI
}
else
{
return CommandException(Resource::String::SingleCharAfterDashError, currArg);
return CommandException(Resource::String::SingleCharAfterDashError(currArg));
}
}
else
Expand Down Expand Up @@ -584,7 +550,7 @@ namespace AppInstaller::CLI
{
if (hasValue)
{
return CommandException(Resource::String::FlagContainAdjoinedError, currArg);
return CommandException(Resource::String::FlagContainAdjoinedError(currArg));
}

m_executionArgs.AddArg(arg.ExecArgType());
Expand All @@ -604,7 +570,7 @@ namespace AppInstaller::CLI

if (!argFound)
{
return CommandException(Resource::String::InvalidNameError, currArg);
return CommandException(Resource::String::InvalidNameError(currArg));
}
}

Expand Down Expand Up @@ -661,36 +627,36 @@ namespace AppInstaller::CLI
{
auto setting = Settings::AdminSettingToString(arg.AdminSetting());
AICLI_LOG(CLI, Error, << "Trying to use argument: " << arg.Name() << " disabled by admin setting " << setting);
throw CommandException(Resource::String::FeatureDisabledByAdminSettingMessage, Utility::LocIndView{ setting }, {});
throw CommandException(Resource::String::FeatureDisabledByAdminSettingMessage(setting));
}

if (!ExperimentalFeature::IsEnabled(arg.Feature()) && execArgs.Contains(arg.ExecArgType()))
{
auto feature = ExperimentalFeature::GetFeature(arg.Feature());
AICLI_LOG(CLI, Error, << "Trying to use argument: " << arg.Name() << " without enabling feature " << feature.JsonName());
throw CommandException(Resource::String::FeatureDisabledMessage, feature.JsonName());
throw CommandException(Resource::String::FeatureDisabledMessage(feature.JsonName()));
}

if (arg.Required() && !execArgs.Contains(arg.ExecArgType()))
{
throw CommandException(Resource::String::RequiredArgError, arg.Name());
throw CommandException(Resource::String::RequiredArgError(arg.Name()));
}

if (arg.Limit() < execArgs.GetCount(arg.ExecArgType()))
{
throw CommandException(Resource::String::TooManyArgError, arg.Name());
throw CommandException(Resource::String::TooManyArgError(arg.Name()));
}
}

if (execArgs.Contains(Execution::Args::Type::Silent) && execArgs.Contains(Execution::Args::Type::Interactive))
{
throw CommandException(Resource::String::TooManyBehaviorsError, s_Command_ArgName_SilentAndInteractive);
throw CommandException(Resource::String::TooManyBehaviorsError(s_Command_ArgName_SilentAndInteractive));
}

if (execArgs.Contains(Execution::Args::Type::CustomHeader) && !execArgs.Contains(Execution::Args::Type::Source) &&
!execArgs.Contains(Execution::Args::Type::SourceName))
{
throw CommandException(Resource::String::HeaderArgumentNotApplicableWithoutSource, Argument::ForType(Execution::Args::Type::CustomHeader).Name());
throw CommandException(Resource::String::HeaderArgumentNotApplicableWithoutSource(Argument::ForType(Execution::Args::Type::CustomHeader).Name()));
}

if (execArgs.Contains(Execution::Args::Type::Count))
Expand Down Expand Up @@ -719,15 +685,17 @@ namespace AppInstaller::CLI
{
applicableArchitectures.emplace_back(Utility::ToString(i));
}
throw CommandException(Resource::String::InvalidArgumentValueError, Argument::ForType(Execution::Args::Type::InstallArchitecture).Name(), std::forward<std::vector<Utility::LocIndString>>((applicableArchitectures)));

auto validOptions = Utility::Join(", "_liv, applicableArchitectures);
throw CommandException(Resource::String::InvalidArgumentValueError(Argument::ForType(Execution::Args::Type::InstallArchitecture).Name(), validOptions));
}
}

if (execArgs.Contains(Execution::Args::Type::Locale))
{
if (!Locale::IsWellFormedBcp47Tag(execArgs.GetArg(Execution::Args::Type::Locale)))
{
throw CommandException(Resource::String::InvalidArgumentValueErrorWithoutValidValues, Argument::ForType(Execution::Args::Type::Locale).Name(), {});
throw CommandException(Resource::String::InvalidArgumentValueErrorWithoutValidValues(Argument::ForType(Execution::Args::Type::Locale).Name()));
}
}

Expand Down
17 changes: 1 addition & 16 deletions src/AppInstallerCLICore/Command.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,10 @@ namespace AppInstaller::CLI
struct CommandException
{
CommandException(Resource::LocString message) : m_message(std::move(message)) {}

// The message should be a localized string.
// The parameters can be either localized or not.
// We 'convert' the param to a localization independent view here if needed.
CommandException(Resource::LocString message, Resource::LocString param) : m_message(std::move(message)), m_params({ param }) {}
CommandException(Resource::LocString message, std::string_view param) : m_message(std::move(message)), m_params({ Utility::LocIndString{ param } }) {}

// The message should be a localized string, but the replacement and parameters are not.
// This supports replacing %1 in the message with the replace value.
CommandException(Resource::LocString message, Utility::LocIndView replace, std::vector<Utility::LocIndString>&& params) :
m_message(std::move(message)), m_replace(replace), m_params(std::move(params)) {}

const Utility::LocIndString Message() const;
const std::vector<Utility::LocIndString>& Params() const { return m_params; }
const Utility::LocIndString Message() const { return m_message; }

private:
Resource::LocString m_message;
std::optional<Utility::LocIndString> m_replace;
std::vector<Utility::LocIndString> m_params;
};

// Flags to control the behavior of the command output.
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCLICore/Commands/FeaturesCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace AppInstaller::CLI
{
table.OutputLine({
std::string{ feature.Name() },
Resource::Loader::Instance().ResolveString(ExperimentalFeature::IsEnabled(feature.GetFeature()) ? Resource::String::FeaturesEnabled : Resource::String::FeaturesDisabled),
Resource::LocString{ ExperimentalFeature::IsEnabled(feature.GetFeature()) ? Resource::String::FeaturesEnabled : Resource::String::FeaturesDisabled},
std::string { feature.JsonName() },
std::string{ feature.Link() } });
}
Expand Down
3 changes: 2 additions & 1 deletion src/AppInstallerCLICore/Commands/InstallCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ namespace AppInstaller::CLI
{
if (ConvertToScopeEnum(execArgs.GetArg(Args::Type::InstallScope)) == Manifest::ScopeEnum::Unknown)
{
throw CommandException(Resource::String::InvalidArgumentValueError, s_ArgumentName_Scope, { "user"_lis, "machine"_lis });
auto validOptions = Utility::Join(", "_liv, std::vector<Utility::LocIndString>{ "user"_lis, "machine"_lis});
throw CommandException(Resource::String::InvalidArgumentValueError(s_ArgumentName_Scope, validOptions));
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/AppInstallerCLICore/Commands/RootCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,15 +187,15 @@ namespace AppInstaller::CLI
info << std::endl <<
"Windows: "_liv << Runtime::GetOSVersion() << std::endl;

info << Resource::String::SystemArchitecture << ": "_liv << Utility::ToString(Utility::GetSystemArchitecture()) << std::endl;
info << Resource::String::SystemArchitecture(Utility::ToString(Utility::GetSystemArchitecture())) << std::endl;

if (Runtime::IsRunningInPackagedContext())
{
info << Resource::String::Package << ": "_liv << Runtime::GetPackageVersion() << std::endl;
info << Resource::String::Package(Runtime::GetPackageVersion()) << std::endl;
};

info << std::endl << Resource::String::Logs << ": "_liv << Runtime::GetPathTo(Runtime::PathName::DefaultLogLocationForDisplay).u8string() << std::endl;
info << std::endl << Resource::String::UserSettings << ": "_liv << UserSettings::SettingsFilePath(true).u8string() << std::endl;
info << std::endl << Resource::String::Logs(Utility::LocIndView{ Runtime::GetPathTo(Runtime::PathName::DefaultLogLocationForDisplay).u8string() }) << std::endl;
info << std::endl << Resource::String::UserSettings(Utility::LocIndView{ UserSettings::SettingsFilePath(true).u8string() }) << std::endl;

info << std::endl;

Expand Down
Loading

0 comments on commit 66b851f

Please sign in to comment.