Skip to content

Conversation

@rikivillalba
Copy link
Contributor

In response to #6613

This is my proposal to automate link generation:

  • The logic is in .translation_links.R in the vignettes directory, and it is sourced in each vignette
  • The message "Translations of this document are available in the following languages" is translated in the same source (there is a switch which defaults to english)
  • Currently the output format is en | fr | es | ..., that is, not a list but all inline.

Note that currently, in documents with a TOC (datatable-faq.Rmd) the link list is below the toc in the packaged vignettes. That is unchanged as the TOC is built automatically by commonmark.
Also, #6394 should be considered.

@codecov
Copy link

codecov bot commented Nov 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.59%. Comparing base (f248d33) to head (b8c1f53).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6618   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files          79       79           
  Lines       14661    14661           
=======================================
  Hits        14455    14455           
  Misses        206      206           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

block = sprintf(
"%s: %s\n",
switch(lang,
fr = "Des traductions de ce document sont disponibles dans les langues suivantes",
Copy link
Member

Choose a reason for hiding this comment

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

This method for defining translations is non-standard. I think this would be better as gettext(...)
or to simplify, just use English. Only list the other languages at the top of the English vignette. and then the other vignettes can just link back to English.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think in gettext however I don't figure out how to get the string for the appropriate language to compile the vignettes in all languages, other than set Sys.setLanguage() in each call.
Another option is to hardcode the header and let the code generate only the links.
But I think your second option is ok.

@rikivillalba
Copy link
Contributor Author

I made the following changes:

  • The script is now is a function to which the "translations available" message is an arg, so it is not harcoded but passed from the vignette text.
  • The message only prints if there are any translations other than current language (as now french version of datatable-joins.Rmd is missing)
  • renamed .translation_links.R to _translation_links.R so ls can see it (it is part of the vignettes after all)
  • NOTE: I manually merge datatable-keys-fast-subset.Rmd, datatable-reference-semantics.Rmd and datatable-secondary-indices-and-auto-indexing.Rmd because Using hyperlinks instead of vignette() calls for readability; doing the same for vignette titles without links for consistency #6617 modified lines adjacent to this branch.

_translation_links.R must be bundled in the package so r CMD check can build the vignettes. However that source is 'knitted' and present in each .R file generated from .Rmd and installed in doc.

French vignettes are no modified at all. An equivalent to "from English original vignette" message linking back to english might be added to translated vignettes, or code similar to that of the english ones, it is up to the owner to decide. In the script case it will add all available links excluding that of the current vignette language determined from the folder name.

As part of spanish translators there is work in progres in https://github.com/cienciadedatos/traduccion-vignettes-datatable
tks

@MichaelChirico MichaelChirico added the translation issues/PRs related to message translation projects label Dec 3, 2024
rikivillalba and others added 2 commits December 16, 2024 13:57
Co-authored-by: Michael Chirico <chiricom@google.com>
Co-authored-by: Michael Chirico <chiricom@google.com>
@rikivillalba
Copy link
Contributor Author

This is the language-link printing function, with the propositions, adapted to litedown. Must be after #6583

# build a link list of alternative languages (may be character(0))
# idea is to look like 'Other languages: en | fr | de'
.write.translation.links <- function(fmt) {
  url = "https://rdatatable.gitlab.io/data.table/articles"
  # TODO: better litedown alternative to knitr::current_input()
  # fuse() setwd the .Rmd dir before evaluating chunks
  input_file = basename(litedown:::.env$input)
  path = dirname(normalizePath(input_file, mustWork = TRUE))
  
  if (basename(path) == "vignettes") {
    lang = "en"
  } else {
    lang = basename(path)
    path = dirname(path)
  }
  translation = dir(path,
    recursive = TRUE,
    pattern = glob2rx(input_file)
  )
  transl_lang = ifelse(dirname(translation) == ".", "en", dirname(translation))
  block = if (!all(transl_lang == lang)) {
    sprintf(fmt, paste(collapse = " | ", ifelse(
      transl_lang != lang,
      sprintf("[%s](%s)", transl_lang,  file.path(url, sub("(?i)\\.Rmd$", ".html", translation))),
      transl_lang     # print no link, only language code.
    )))
  } else ""
  block
}

@MichaelChirico
Copy link
Member

Filed yihui/litedown#67 requesting a public interface equivalent to knitr::current_input()

Co-authored-by: Ricardo Demián Villalba <32423469+rikivillalba@users.noreply.github.com>
@aitap
Copy link
Member

aitap commented Feb 17, 2025

My apologies to everyone involved for pushing e7e966e straight to master. I forgot to create a branch first.

Does this PR need any additional work, besides cleaning up the merge conflicts, before it can be merged?

@rikivillalba
Copy link
Contributor Author

My apologies to everyone involved for pushing e7e966e straight to master. I forgot to create a branch first.

Does this PR need any additional work, besides cleaning up the merge conflicts, before it can be merged?

I think this pr is linked to #6583, and to the litedown feature akin to knitr::current_input(): litedown::get_context("input") not yet in CRAN.

@MichaelChirico
Copy link
Member

I think this pr is linked to #6583, and to the litedown feature akin to knitr::current_input(): litedown::get_context("input") not yet in CRAN.

I don't think that's required to merge here. If you think this is ready in its current form, let's merge, we can mark the litedown translation part for follow-up.

@rikivillalba
Copy link
Contributor Author

rikivillalba commented Mar 3, 2025

I think this pr is linked to #6583, and to the litedown feature akin to knitr::current_input(): litedown::get_context("input") not yet in CRAN.

I don't think that's required to merge here. If you think this is ready in its current form, let's merge, we can mark the litedown translation part for follow-up.

Added the code sections to generate the translation links. this is the status:

What was done

  • All vignettes currently on master are updated, I merged the most recent content and added the code to russian and french vignettes for ease of merging.
  • Building the articles via pkgdown::build_articles() works as expected.

Pending:

  • The message header for translation links is not translated yet (i'm handing off to translators). The english is "Translations of this document are available in: %s". Using gettext here would imply changing locale before running each vignette, someting I think is not worth and I didn' tested.
    • Should the advice be kept in English in all vignettes?

Observations:

  • Merging should probably be manually solved for the code printing the links being just aside the original links.

@rikivillalba rikivillalba marked this pull request as ready for review March 4, 2025 14:26
@rikivillalba rikivillalba requested review from a team and jangorecki as code owners March 4, 2025 14:26
Copy link
Member

@aitap aitap left a comment

Choose a reason for hiding this comment

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

Thank you! I'm fine with or without translation of "this document also available in:".

Copy link
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

I think this is good to go :shipit:

We can iterate more later.

@MichaelChirico MichaelChirico merged commit df59c7d into master Mar 4, 2025
8 of 10 checks passed
@MichaelChirico MichaelChirico deleted the automate-lang-links branch March 4, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

translation issues/PRs related to message translation projects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants