-
Notifications
You must be signed in to change notification settings - Fork 492
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
Prepare for language translations #2242
base: feature/i18n
Are you sure you want to change the base?
Conversation
in version.texi.in
according to recommendations from https://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings
@@ -66,7 +66,6 @@ m4/ | |||
make.sh | |||
missing | |||
mkinstalldirs | |||
po/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we okay with re-introducing the po/
directory again? If not what would be a preferred location in the repository?
@@ -3,3 +3,4 @@ set(Ledger_VERSION_MINOR 3) | |||
set(Ledger_VERSION_PATCH 2) | |||
set(Ledger_VERSION_PRERELEASE "") | |||
set(Ledger_VERSION_DATE 20230330) | |||
set(Ledger_VERSION "${Ledger_VERSION_MAJOR}.${Ledger_VERSION_MINOR}.${Ledger_VERSION_PATCH}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for convenience, as much as I enjoy typing it gets tedious at some point.
--keyword='_' | ||
--keyword='_f' | ||
--keyword='_n:1,2' | ||
--keyword='_fn:1,2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keywords refer to the special custom macros for gettext functions defined in system.hh.in
:200ff
@@ -0,0 +1,55 @@ | |||
# Support for International Languages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewers: What is missing from this file that would be helpful for developers, collaborators, translators that wish to update Ledger's codebase and/or translations?
@@ -103,7 +103,8 @@ class account_t : public flags::supports_flags<>, public scope_t | |||
virtual ~account_t(); | |||
|
|||
virtual string description() { | |||
return string(_("account ")) + fullname(); | |||
// TRANSLATORS: %1% refers to the full name of the account |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These special TRANSLATORS
comments show up in the .po
files and provide context for translators. Ideally this is done for the majority of the messages, but as this is quite an effort I wanted to get 👀 on these changes to verify that this is headed into the right direction.
|
||
if (post.amount) | ||
out << _(" Amount: ") << *post.amount << std::endl; | ||
out << indent << _f("Amount: %1%") % *post.amount << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the indentation should not be part of the translatable string; do people dis/agree?
#if HAVE_BOOST_PYTHON | ||
<< _("and with Python support") | ||
#else | ||
<< _("and without Python support") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how the message for Ledger's gpg and Python capabilities is pieced togeher.
Neovim's output with the -v
option includes a "Features" line, e.g. Features: +acl +iconv +tui
. For Ledger this could be: Features: +gpg +py
is this an improvement over the current output?
// TRANSLATORS: This is an error message when the gpgme encryption library | ||
// could not be initialized. | ||
// %1% refers to the source of the error | ||
// %2% refers to the actual error as a string. | ||
throw_(runtime_error, _f("%1%: %2%") % err.source() % err.asString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be changed to something like:
throw_(runtime_error, _f("%1%: %2%") % err.source() % err.asString()); | |
throw_(runtime_error, _f("Unable to initalize GPG at %1%: %2%") % err.source() % err.asString()); |
// %2% refers to the value of the metadata | ||
// %3% [Unknown] | ||
_f("Metadata assertion failed for (%1%: %2%): %3%") | ||
% key % value % (*i).second.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwiegley What does (*i).second.first)
refer to?
::textdomain("ledger"); | ||
::setlocale(LC_MESSAGES, ""); | ||
::bindtextdomain(GETTEXT_DOMAIN, GETTEXT_LOCALE_DIR); | ||
::bind_textdomain_codeset(GETTEXT_DOMAIN, "UTF-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to always use the UTF-8 codeset?
_("Construct an amount object whose display precision is always equal to its\n\ | ||
internal precision.")) | ||
"Construct an amount object whose display precision is always equal" | ||
"to its\ninternal precision.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe DocStrings should be in English and not be translated.
.staticmethod("exact") | ||
|
||
.def(init<amount_t>()) | ||
|
||
.def("compare", &amount_t::compare, args("amount"), | ||
_("Compare two amounts for equality, returning <0, 0 or >0.")) | ||
"Compare two amounts for equality, returning <0, 0 or >0.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v.s.
@@ -55,61 +55,58 @@ value_t report_statistics(call_scope_t& args) | |||
assert(is_valid(statistics.earliest_post)); | |||
assert(is_valid(statistics.latest_post)); | |||
|
|||
out << format(_f("Time period: %1% to %2% (%3% days)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwiegley Can you thing of a reason why the outer format
may have been significant?
@@ -1341,7 +1341,7 @@ void value_t::in_place_not() | |||
break; | |||
} | |||
|
|||
add_error_context(_f("While applying not to %1%:") % *this); | |||
add_error_context(_f("While applying 'not' to %1%:") % *this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwiegley The quoting within Ledger's messages is a bit inconsistent. From what I can tell most messages use single quotes and a few double quotes.
This PR maintains compatibility with previous versions.
Are you okay with deciding on one type of quotes (my preferences: 'single') and change all the double quotes to single quotes within Ledger's messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think there are a few messages that refer to a ledger key word (like not
here), but do not quote it. I believe being consistent here and quoting all Ledger keywords will be helpful for users and translators.
if(CMAKE_INSTALL_LOCALEDIR) | ||
set(GETTEXT_LOCALE_DIR "${CMAKE_INSTALL_LOCALEDIR}") | ||
else() | ||
set(GETTEXT_LOCALE_DIR "FIXME") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a sensible default here?
set(GETTEXT_LOCALE_DIR "FIXME") | |
set(GETTEXT_LOCALE_DIR "locale") |
return empty_string; | ||
|
||
assert(len < 1024 * 1024); | ||
|
||
std::ostringstream out; | ||
|
||
if (item.pos->pathname.empty()) { | ||
out << desc << _(" from streamed input:"); | ||
// FIXME: Find out what `desc` represents and update comment for translators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwiegley Can you provide more information what values desc
can hold? I assume the variable name is an abbreviation of description
, yet that does not tell a translator much about how to best fit this piece into a sentence…
This PR adds support for international languages by fully enabling gettext support in Ledger.
Since this is a larger PR I'll tag is as a draft, so folks can chime in and provide feedback.
The changes are widespread throughout the codebase as the way that Ledger's creates its output messages had to change. Please review the inline comments on this PR for questions, details of note, and further context on the thinking that went into this PR.
Some of Ledgers messages were pieced together in output streams, yet to better support translations to languages that have a grammar that is very different from English or similar Roman languages the use of format strings is preferred. Other messages would benefit from comments made available to translators as to provide
more context about the message and its placeholder parameters.
Example:
src/draft.cc
out << _("Date: ") << *date << std::endl;
out << _f("Date: %1%") % *date << std::endl;
Closes #1241
TODO:
☝️P.S.: This is the result of a few days work on many areas of the code, hence I kindly request thorough review should something have slipped my attention, while wrangling all the necessary changes to make this a reality.
✌️P.P.S.:
separate PR will follow that adds the first international language to Ledger.See #2243