Skip to content
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

ENH - Update translations workflows #1959

Merged
merged 41 commits into from
Sep 11, 2024

Conversation

trallard
Copy link
Collaborator

@trallard trallard commented Aug 19, 2024

This PR supersedes #1294 and is the last missing piece to get rid of nox and the `noxfile.

Summary:

  • Adds all the needed babel commands to our tox.ini file
  • Updates both user and contributor documentation
  • Removes unnecessary bits from the CI workflows

Questions:

@trallard trallard added kind: enhancement New feature or request kind: documentation Improvements or additions to documentation tag: i18N Items related to internationalization labels Aug 19, 2024
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

this is great progress! Some comments on the doc changes

docs/community/topics/i18n.rst Outdated Show resolved Hide resolved
docs/community/topics/i18n.rst Outdated Show resolved Hide resolved
Comment on lines 54 to 60
tox run -e i18n-extract

pybabel extract . -F babel.cfg -o src/pydata_sphinx_theme/locale/sphinx.pot -k '_ __ l_ lazy_gettext'

**To run this in ``.nox``**: ``nox -s translate -- extract``.

#. Update the message catalogs (``PO`` files) with `the PyBabel update command <https://babel.pocoo.org/en/latest/cmdline.html#update>`__:
#. Update the message catalogs (``PO`` files) for the existing locales:

.. code-block:: bash

pybabel update -i src/pydata_sphinx_theme/locale/sphinx.pot -d src/pydata_sphinx_theme/locale -D sphinx

**To run this in ``.nox``**: ``nox -s translate -- update``.
tox run -e i18n-update
Copy link
Collaborator

Choose a reason for hiding this comment

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

since the workflow is

tox run -e i18n-extract
tox run -e i18n-update

...with apparently no steps in between, I find myself wondering why these are two separate steps (from the contributor perspective). Why not a single tox command to do both things?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤔 all the sources and projects I surveyed to get an idea of the workflow treat these as a separate step... will have another thought and it might make sense to merge

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, I guess it makes sense sometimes to do update without doing extract (i.e., if only non-translatable stuff changed, and you just want to update the line numbers)? But even so it would still be possible to have a single recipe to do both, and a sub-recipe that only did the update part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok I just merged extract and update so this is now part of the same command/step

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes total sense to me based on the workflow I followed.

docs/community/topics/i18n.rst Outdated Show resolved Hide resolved
docs/community/topics/i18n.rst Outdated Show resolved Hide resolved
docs/community/topics/i18n.rst Show resolved Hide resolved
docs/community/topics/i18n.rst Outdated Show resolved Hide resolved
Comment on lines 143 to 144
Translation files
------------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

some of the info in this section (e.g., "there are 3 types of files, here's what they are") would have been useful context at the beginning of the page... but some of it (e.g., details about #: vs #,) might be too "in the weeds" to come right at the beginning. Consider splitting up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a section I struggled with -- content and position -- so perhaps splitting would be good.

I think the too detailed bits can go under the "Tips" section...

docs/user_guide/i18n.rst Outdated Show resolved Hide resolved
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

@Carreau Carreau force-pushed the trallard/update-translations branch from 189de0e to b3dacbd Compare August 28, 2024 09:16
@gabalafou
Copy link
Collaborator

Thanks for the explanation/clarification :)

@trallard
Copy link
Collaborator Author

trallard commented Aug 30, 2024

It also seems that the Transifex integration can be configured to automatically pull changes from our repo https://help.transifex.com/en/articles/6265125-github-installation-and-configuration

That is the only question remaining for @12rambau and I don't think that is a blocker as we could in any case update the files manually later. So aside from that the PR is ready now I believe
so whenever any of @Carreau or @gabalafou can review again and approve if applicable we could merge it seems.

Comment on lines +142 to +144
docs-live-theme: stb serve docs --open-browser --re-ignore=locale|api|_build|\.jupyterlite\.doit\.db
docs-live: sphinx-autobuild docs/ docs/_build/html --open-browser --re-ignore=locale|api|_build|\.jupyterlite\.doit\.db
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got annoyed at the docs live command rebuilding also the JS/CSS assets when working only in docs so I split it here.

Co-authored-by: Daniel McCloy <dan@mccloy.info>
Copy link
Collaborator

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

I noticed something that looks off

Copy link
Collaborator

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

I noticed something that looks off

#. Sign up for a `Transifex account <https://www.transifex.com/signup/>`__.
#. Join the `PyData Sphinx Theme project <https://explore.transifex.com/12rambau/pydata-sphinx-theme/>`__.
#. Select the language you want to localize. If the language you are looking for is not listed, you can `open an issue on GitHub to request it <https://github.com/pydata/pydata-sphinx-theme/issues>`__. Then you can open a pull request
to add the new language following the steps outlined in :ref:`adding-new-language`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to revert as the previous one was making the docs build fail

@trallard trallard merged commit 721d197 into pydata:main Sep 11, 2024
29 checks passed
@trallard trallard deleted the trallard/update-translations branch September 11, 2024 10:21
@Carreau
Copy link
Collaborator

Carreau commented Sep 11, 2024

Thanks !

trallard added a commit that referenced this pull request Nov 25, 2024
While investigating
#2040 I noticed that
our current release missed the updated `.mo` files.
This was a bug I introduced in
#1959 when reworking
the localisation workflows.

TLD;R—Since we use `stb` to build the theme, I did not realise that
compiling the i18n files had to be done within the `stb package` (I
removed it from the webpack file as this was causing the infinite reload
while working on our docs).

Since this was an easy miss, I added our build and inspect job to the
`pre-release` workflow that runs on a chron job to check our build
process periodically.

Once we merge this, we can make a small release to patch the current
issue.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: gabalafou <gabriel@fouasnon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: documentation Improvements or additions to documentation kind: enhancement New feature or request tag: i18N Items related to internationalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants