Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement po_create()/po_update() for creating/updating translations #235
Implement po_create()/po_update() for creating/updating translations #235
Changes from 17 commits
3affcc0
63dcbc5
cfe1eff
b205c34
6124465
c644cb7
a088663
28f3e28
310f2b0
10e16fd
42885b1
205ee8f
0d3391d
d8880e0
efa92b6
511fad1
d7c3882
0daabcd
b604281
ef5449f
7322e55
78afb31
7133453
df99ea4
f081b01
6fd0c30
0e4371c
4b3353f
ae5c5b7
b4e71bd
b8e17e9
0f93068
3ff40d5
5efd88f
e6788bd
a36031a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we clarify what happens to existing translations here too? Details may be overkill, but mentioning fuzzying seems appropriate.
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'll put most of the details in
po_update()
and then link to there from here.I think the updating here is mostly incidental, just because if you use
msginit
without checking, it will reset the existing translations, which I don't think you ever want. But at the same time, the main point of this function is not the updating, so I don't want to draw too much attention to it.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.
it might be nice to
warn()
here if e.g. R-pkg.pot is present but pkg.pot is not & the package has asrc/
directory.OTOH, if the translation is handled entirely by
potools
, this should only happen ifsrc/
doesn't have any messages to translate, right? So maybe it will be a warning highly prone to false positives...skipping for now, but file a follow-up issue if you think the warning is worthwhile.
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 one place this will break is for
base
, which usesR-base.pot
for R andR.pot
for src. I'll just add a TODO for now to make sure that case is handled later.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.
love the look of this, and looks like a great redesign of generating test packages (the current suite is pretty unwieldy/arbitrarily organized). a good companion would be
local_translation_conn()
to keep translations right next to where they'll be used too.but feels like it should be in
tests/testthat/helper.R
, no?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.
We've generally been moving away from using helper files, because it seems more natural just to keep all code in one place. Additionally, as these
local_
type helpers grow more complex, it's not crazy that you might also want to test them.That said, I don't feel particularly strongly about it, and since you already have
helper.R
, that's a more sensible home, so I'll move it there.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'm not sure why this was CHARSET before, but I was getting an error from
tools::checkPoFile()
that it was failing toiconv
CHARSET
toUTF-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.
Hmm note that .pot is not mentioned in
?tools::checkPoFile
.CHARSET
shows up in templates in base:https://github.com/wch/r-source/blob/bf5885960808d7c539e74b238575f54de0faba04/src/library/base/po/R.pot#L17
https://github.com/wch/r-source/blob/bf5885960808d7c539e74b238575f54de0faba04/src/library/base/po/R-base.pot#L10
Here's what I'm seeing in the
xgettext
docs:https://www.gnu.org/software/gettext/manual/gettext.html
and
In the gettext sources, here is where
xgettext
initially populates the defaultCHARSET
:https://github.com/autotools-mirror/gettext/blob/030c0341a4a0ba6ad7fe62e83ff663bdc76cbe4d/gettext-tools/src/xgettext.c#L2015-L2028
Here
write-po
treatsCHARSET
as ASCII:https://github.com/autotools-mirror/gettext/blob/030c0341a4a0ba6ad7fe62e83ff663bdc76cbe4d/gettext-tools/src/write-po.c#L1650-L1653
This snippet makes me thing setting UTF-8 is probably OK, since the default
CHARSET
has the probably-in-general faulty assumption that the strings in the .pot file are all-ASCII:https://github.com/autotools-mirror/gettext/blob/030c0341a4a0ba6ad7fe62e83ff663bdc76cbe4d/gettext-tools/src/po-charset.c#L490-L509
That said, current r-devel .pot are all-ASCII:
CRAN would require this to hold true for all packages at the R level, and probably the C level?
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.
We could probably patch
tools::checkPoTools
to accept theCHARSET
➡️ASCII
logic, but I'm not sure that would solve all of the issues of usingcheckPoTools()
on .pot files when it was designed for .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.
Maybe I'll back this change out here and we can discuss more thoroughly in a separate issue. I think potools should assume all
.pot
and.po
files are UTF-8 and then fix any downstream problems that causes.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.
Hmmm, backing out this change doesn't break any of my tests (or interactive usage) so definitely doesn't need to be here.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 copied this from #257, but it feels a bit wordy to me. What do you think about not showing the call by default?
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.
Hmm it's coming from
verbose=TRUE
(now the default), are you suggesting we flip the default, or that we implementverbose=0,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.
I don't think it's necessary to routinely display a message for success. Adding levels of verbosity is unlikely to be worth the effort IMO, so I'd be in favour of
verbose = FALSE
by default and not passing inverbose
from the wrapping function.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.
Oh that's not quite right; I think on success we want to see the stdout/stderr from the command line tool, but not the call.
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've tweaked the code to behave the way that I want just to give you a concrete proposal to look at. Let me know what you think.