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

Support translated notebooks #600

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

OriolAbril
Copy link
Contributor

Closes #587. The issue points to the relevant sphinx source,
where it can be seen sphinx "marks" translated strings with <translated> at the
end of the source_path argument, which is then stored as the current_source attribute of
the docutils.nodes.document class, which is the second argument of Parser.parse.

This adds an extra condition to fall back to myst-parser which allows using myst-nb with translated
content.

It would be nice to add some test for this but I am not familiar enough with the test suite yet,
I'll try to add one in the coming days. It can probably reuse one of the existing notebooks,
but it will be necessary to add a locale folder with the translations, will it be ok to add
it directly in the tests folder?

So far I have only checked it works locally on a couple repos I had
(both executing and not executing notebooks).

@agoose77
Copy link
Collaborator

I won't include this in the current patch release, as I don't have enough bandwidth to avoid blocking the Jupyter Book release that needs to follow. I'll circle back around to this PR soon :)

@bsipocz
Copy link
Collaborator

bsipocz commented Sep 20, 2024

@OriolAbril - Adding a test here would be really useful. I would a translated version of a test notebook should be sufficient.
I would think adding the locale in the tests folder should be fine as long as the test is being picked up and pytest doesn't start complaining about it.

@bsipocz bsipocz added the enhancement New feature or request label Sep 20, 2024
@OriolAbril
Copy link
Contributor Author

I did my best to modify one of the tests in test_parser to use translations.

@OriolAbril
Copy link
Contributor Author

It seems different versions of sphinx generate different translation related metadata and the xml comparison fails. I don't know how to fix that

@bsipocz
Copy link
Collaborator

bsipocz commented Sep 23, 2024

It seems different versions of sphinx generate different translation related metadata and the xml comparison fails. I don't know how to fix that

I have some open PRs that revisit the test requirements, as well as planning to do another cleanup of the versions in GHA. One those are done and this still runs into CI issue, we can revisit with some version dependent conditionals, or just state that translations support will require a newer sphinx than the stated minimum supported one.

@OriolAbril
Copy link
Contributor Author

The way the PR introduces to handle translations works in all versions. However, the html/xml metadata related to translations differs for the different versions. The xml generated with sphinx 8 which is the one I added to the tests folder is the following:

<document source="basic_run_intl" translation_progress="{'total': 4, 'translated': 2}">
    <section ids="a-title" names="a\ title un\ título">
        <title translated="True">
            un título
        <paragraph translated="True">
            algo de texto
        <container cell_index="1" cell_metadata="{}" classes="cell" exec_count="1" nb_element="cell_code">
            <container classes="cell_input" nb_element="cell_code_source">
                <literal_block language="ipython3" xml:space="preserve">
                    a=1
                    print(a)
            <container classes="cell_output" nb_element="cell_code_output">
                <literal_block classes="output stream" language="myst-ansi" xml:space="preserve">
                    1

In sphinx<8 the translated="True" in title and paragraph elements and translation_progress="{'total': 4, 'translated': 2}" in the document element are not there. All of them have the translated content properly injected and rendered though.

If it were possible to ignore this metadata when comparing against the xml output then tests would pass for all versions, and would still check that the actual translated content is injected together.

@bsipocz
Copy link
Collaborator

bsipocz commented Sep 25, 2024

@OriolAbril - Experimenting and combing the code a little bit through I'm fairly certain that the logic is the opposite: we need the old sphinx-generated file for comparison and then ignore the extra bit of metadata in conftest. There are already a couple of similar cases.

As I was iterating on this locally, I've pushed the changes directly to your branch.

I think this is good to go now, but I'll leave this open so @agoose77 can double-check it, too.

@OriolAbril
Copy link
Contributor Author

Thanks @bsipocz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not work with Sphinx's internationalization workflow
3 participants