Implement po_create()/po_update() for creating/updating translations #235

hadley · 2021-11-01T15:01:19Z

I don't love the name tr_add(), and it's making me doubt that the developer/translator distinction is that useful, because this function might be run by the developer or an R-packages savvy translator.

I tweaked the .Rproj to make my usual roxygen2 workflow a little smoother; let me know if you want to revert.

I'm little surprised that potools doesn't already use msginit, so maybe I should instead copy the approach used in translate_package()?

R/msgmerge.R

hadley · 2021-11-01T15:12:43Z

Changed it to add or update, depending on whether or not the .po file already exists. That makes tr_add() and even worse name.

hadley · 2021-11-04T13:20:17Z

I think this is better as po_create() and po_update(). I think po_update() should update all languages if none supplied; but that needs code from #234, so I'll wait until you've have a chance to review that.

potools.Rproj

MichaelChirico · 2021-11-06T22:11:12Z

I'm little surprised that potools doesn't already use msginit

There are a couple of gettext tools I would do well to integrate better with / provide good wrappers for...

MichaelChirico · 2021-11-06T22:18:17Z

R/msgmerge.R

+#'   new messages are added; and translations for deleted message are marked
+#'   as deprecated and moved to the bottom of the file.
+#'
+#' @param lang Language identifiers. These are typically two letters (e.g.


This basically repeats the description in ?translate_package... @inheritParams doesn't feel quite right because of how detailed we want to be... Maybe we need a ?language-code .Rd file we can refer to? @param languages Language identifiers. See [language-code] for details.?

R/msgmerge.R

Conflicts: DESCRIPTION NAMESPACE

Merge commit '3849f477c94b95af34764c5c96b7c0fce284eedd'

And fundamentally change approach

hadley · 2021-11-09T17:02:33Z

Ok, reimplemented basically from scratch; I'll add a bunch of tests shortly.

Bringing in system2() code from #257

And fix the bugs thus revealed

hadley · 2021-11-09T20:09:35Z

tests/testthat/_snaps/po_create.md

+      po_create("jp", verbose = TRUE)
+    Message <simpleMessage>
+      Updating 'jp' R translation
+      Running msgmerge on './po/R-jp.po' succeeded:


I copied this from #257, but it feels a bit wordy to me. What do you think about not showing the call by default?

Hmm it's coming from verbose=TRUE (now the default), are you suggesting we flip the default, or that we implement verbose=0,1,2,...?

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 in verbose from the wrapping function.

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.

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.

hadley · 2021-11-09T20:11:01Z

R/write_po_file.R

@@ -300,7 +300,7 @@ format.po_metadata = function(x, template = FALSE, use_plurals = FALSE, ...) {
    x$email = "EMAIL@ADDRESS"
    x$language = ''
    x$language_team = "LANGUAGE <[email protected]>"
-    x$charset = 'CHARSET'
+    x$charset = 'UTF-8'


I'm not sure why this was CHARSET before, but I was getting an error from tools::checkPoFile() that it was failing to iconv CHARSET to UTF-8.

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

MIME-Version, Content-Type, Content-Transfer-Encoding
These values are set according to the content of the POT file and the current locale. If the POT file contains charset=UTF-8, it means that the POT file contains non-ASCII characters, and we keep the UTF-8 encoding. Otherwise, when the POT file is plain ASCII, we use the locale’s encoding.

and

Content-Type
Replace ‘CHARSET’ with the character encoding used for your language, in your locale, or UTF-8. This field is needed for correct operation of the msgmerge and msgfmt programs, as well as for users whose locale’s character encoding differs from yours (see Charset conversion).

In the gettext sources, here is where xgettext initially populates the default CHARSET:

https://github.com/autotools-mirror/gettext/blob/030c0341a4a0ba6ad7fe62e83ff663bdc76cbe4d/gettext-tools/src/xgettext.c#L2015-L2028

Here write-po treats CHARSET 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:

grep -rP "[\x80-\xFF]" ~/svn/R-devel/ --include=*.pot # <empty>

CRAN would require this to hold true for all packages at the R level, and probably the C level?

We could probably patch tools::checkPoTools to accept the CHARSET ➡️ ASCII logic, but I'm not sure that would solve all of the issues of using checkPoTools() on .pot files when it was designed for .po.

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.

Hmmm, backing out this change doesn't break any of my tests (or interactive usage) so definitely doesn't need to be here.

R/msgmerge.R

MichaelChirico · 2021-11-10T01:48:21Z

R/po_create.R

+#'
+#' `po_create()` creates a new `po/{languages}.po` containing the messages to be
+#' translated. If a translation already exists, it'll be updated with any
+#' changes to the `.pot` since it was last touched.


Can we clarify what happens to existing translations here too? Details may be overkill, but mentioning fuzzying seems appropriate.

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.

R/msgmerge.R

MichaelChirico · 2021-11-10T02:18:34Z

R/write_po_file.R

@@ -300,7 +300,7 @@ format.po_metadata = function(x, template = FALSE, use_plurals = FALSE, ...) {
    x$email = "EMAIL@ADDRESS"
    x$language = ''
    x$language_team = "LANGUAGE <[email protected]>"
-    x$charset = 'CHARSET'
+    x$charset = 'UTF-8'


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

MIME-Version, Content-Type, Content-Transfer-Encoding
These values are set according to the content of the POT file and the current locale. If the POT file contains charset=UTF-8, it means that the POT file contains non-ASCII characters, and we keep the UTF-8 encoding. Otherwise, when the POT file is plain ASCII, we use the locale’s encoding.

and

Content-Type
Replace ‘CHARSET’ with the character encoding used for your language, in your locale, or UTF-8. This field is needed for correct operation of the msgmerge and msgfmt programs, as well as for users whose locale’s character encoding differs from yours (see Charset conversion).

In the gettext sources, here is where xgettext initially populates the default CHARSET:

https://github.com/autotools-mirror/gettext/blob/030c0341a4a0ba6ad7fe62e83ff663bdc76cbe4d/gettext-tools/src/xgettext.c#L2015-L2028

Here write-po treats CHARSET 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:

grep -rP "[\x80-\xFF]" ~/svn/R-devel/ --include=*.pot # <empty>

CRAN would require this to hold true for all packages at the R level, and probably the C level?

MichaelChirico · 2021-11-10T02:19:41Z

R/write_po_file.R

@@ -300,7 +300,7 @@ format.po_metadata = function(x, template = FALSE, use_plurals = FALSE, ...) {
    x$email = "EMAIL@ADDRESS"
    x$language = ''
    x$language_team = "LANGUAGE <[email protected]>"
-    x$charset = 'CHARSET'
+    x$charset = 'UTF-8'


We could probably patch tools::checkPoTools to accept the CHARSET ➡️ ASCII logic, but I'm not sure that would solve all of the issues of using checkPoTools() on .pot files when it was designed for .po.

MichaelChirico · 2021-11-10T02:21:49Z

tests/testthat/_snaps/po_create.md

+      po_create("jp", verbose = TRUE)
+    Message <simpleMessage>
+      Updating 'jp' R translation
+      Running msgmerge on './po/R-jp.po' succeeded:


Hmm it's coming from verbose=TRUE (now the default), are you suggesting we flip the default, or that we implement verbose=0,1,2,...?

MichaelChirico · 2021-11-10T02:28:50Z

R/utils.R

@@ -167,3 +167,19 @@ is_outdated <- function(src, dst) {
 }

 is_testing = function() identical(Sys.getenv("TESTTHAT"), "true")
+
+local_test_package <- function(..., .envir = parent.frame()) {


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?

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.

Conflicts: NAMESPACE R/po_compile.R R/po_extract.R man/po_compile.Rd man/po_extract.Rd

MichaelChirico · 2021-11-11T02:48:47Z

R/po_create.R

+  }
+
+}
+po_prefix <- function(type = c("R", "src")) {


the one place this will break is for base, which uses R-base.pot for R and R.pot for src. I'll just add a TODO for now to make sure that case is handled later.

MichaelChirico · 2021-11-11T02:53:20Z

R/po_create.R

+#' @export
+po_create <- function(languages, dir = ".", verbose = !is_testing()) {
+  package <- get_desc_data(dir, "Package")
+  po_files <- po_language_files(languages, dir)


it might be nice to warn() here if e.g. R-pkg.pot is present but pkg.pot is not & the package has a src/ directory.

OTOH, if the translation is handled entirely by potools, this should only happen if src/ 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.

MichaelChirico · 2021-11-11T03:17:15Z

Looking great, thanks!

MichaelChirico and others added 2 commits November 1, 2021 09:58

Implement tr_add() for adding new translations

3affcc0

Add or update as necessary

63dcbc5

hadley commented Nov 1, 2021

View reviewed changes

R/msgmerge.R Outdated Show resolved Hide resolved

hadley added 3 commits November 1, 2021 10:14

Only add previous for tr_add()

cfe1eff

Fix typo

b205c34

Split tr_add() into po_create() and po_update()

6124465

hadley changed the title ~~Implement tr_add() for adding new translations~~ Implement po_create()/po_update() for creating/updating translations Nov 4, 2021

hadley mentioned this pull request Nov 6, 2021

potools::translate_package(languages = "sv") with existing "custom_translation_functions"? #240

Open

MichaelChirico reviewed Nov 6, 2021

View reviewed changes

potools.Rproj Outdated Show resolved Hide resolved

MichaelChirico reviewed Nov 6, 2021

View reviewed changes

MichaelChirico added 2 commits November 6, 2021 15:19

lang->languages

c644cb7

lang->languages

a088663

This comment has been minimized.

Sign in to view

MichaelChirico reviewed Nov 6, 2021

View reviewed changes

R/msgmerge.R Outdated Show resolved Hide resolved

MichaelChirico reviewed Nov 6, 2021

View reviewed changes

R/msgmerge.R Outdated Show resolved Hide resolved

hadley added 5 commits November 9, 2021 07:15

Merge commit 'bd9c91a2abc717fe52b56cf6879f83f233a299a1'

28f3e28

Conflicts: DESCRIPTION NAMESPACE

Move to own file

310f2b0

Merge commit '3849f477c94b95af34764c5c96b7c0fce284eedd'

Mark .pot file as UTF-8

10e16fd

Split po_create() and po_update() into pieces

42885b1

And fundamentally change approach

Fix broken tests

205ee8f

hadley added 5 commits November 9, 2021 12:58

Add test for po_create()

0d3391d

Bringing in system2() code from #257

WS

d8880e0

Extract out local_test_package() helper

efa92b6

Extract & test po_language_files()

511fad1

Add tests for create and update

d7c3882

And fix the bugs thus revealed

hadley commented Nov 9, 2021

View reviewed changes

hadley requested a review from MichaelChirico November 9, 2021 20:12

Add missing line

0daabcd

MichaelChirico reviewed Nov 10, 2021

View reviewed changes

R/msgmerge.R Outdated Show resolved Hide resolved

might as well use fifelse

b604281

MichaelChirico reviewed Nov 10, 2021

View reviewed changes

hadley added 12 commits November 10, 2021 06:05

Add links to solaris docs

ef5449f

Revert unintentional change

7322e55

Move local_test_package() to better home

78afb31

Revert CHARSET -> UTF-8 change

7133453

Merge commit '3eaf727e03b1238d01492b99f58f938587b4b73b'

df99ea4

Conflicts: NAMESPACE R/po_compile.R R/po_extract.R man/po_compile.Rd man/po_extract.Rd

Merged origin/add-translation into add-translation

f081b01

More docs about updating

6fd0c30

Standardise number of dots

0e4371c

Improve docs

4b3353f

Tweak messaging

ae5c5b7

Merge branch 'master' into add-translation

b4e71bd

Revert accidental doc changes

b8e17e9

MichaelChirico reviewed Nov 11, 2021

View reviewed changes

MichaelChirico added 2 commits November 10, 2021 18:49

add TODO

0f93068

another TODO

3ff40d5

MichaelChirico reviewed Nov 11, 2021

View reviewed changes

MichaelChirico added 3 commits November 10, 2021 18:58

typo

5efd88f

clarify fuzzy description

e6788bd

comment need for standardise_dots & americanize 🇺🇸

a36031a

MichaelChirico merged commit e9f85ff into MichaelChirico:master Nov 11, 2021

hadley deleted the add-translation branch November 11, 2021 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement po_create()/po_update() for creating/updating translations #235

Implement po_create()/po_update() for creating/updating translations #235

hadley commented Nov 1, 2021

hadley commented Nov 1, 2021

hadley commented Nov 4, 2021

MichaelChirico commented Nov 6, 2021

MichaelChirico Nov 6, 2021

This comment has been minimized.

hadley commented Nov 9, 2021

hadley Nov 9, 2021

MichaelChirico Nov 10, 2021

hadley Nov 10, 2021

hadley Nov 10, 2021

hadley Nov 10, 2021

hadley Nov 9, 2021

MichaelChirico Nov 10, 2021

MichaelChirico Nov 10, 2021

hadley Nov 10, 2021

hadley Nov 10, 2021

MichaelChirico Nov 10, 2021

hadley Nov 10, 2021

MichaelChirico Nov 10, 2021

MichaelChirico Nov 10, 2021

MichaelChirico Nov 10, 2021

MichaelChirico Nov 10, 2021

hadley Nov 10, 2021

MichaelChirico Nov 11, 2021

MichaelChirico Nov 11, 2021

MichaelChirico commented Nov 11, 2021

Implement po_create()/po_update() for creating/updating translations #235

Implement po_create()/po_update() for creating/updating translations #235

Conversation

hadley commented Nov 1, 2021

hadley commented Nov 1, 2021

hadley commented Nov 4, 2021

MichaelChirico commented Nov 6, 2021

Choose a reason for hiding this comment

This comment has been minimized.

hadley commented Nov 9, 2021

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

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

MichaelChirico commented Nov 11, 2021