Skip to content

Conversation

@kinow
Copy link
Member

@kinow kinow commented Jan 26, 2023

Fixes #368

This fixes the build and produces the expected Markdown.

The diff applied to MyST (created on the MyST parser git clone, posting here to show how much I had to change - bugs of few lines are always the ones that take longer to be solved):

diff --git a/myst_parser/parsers/sphinx_.py b/myst_parser/parsers/sphinx_.py
index 94d5aef..2be1d77 100644
--- a/myst_parser/parsers/sphinx_.py
+++ b/myst_parser/parsers/sphinx_.py
@@ -66,5 +66,15 @@ class MystParser(SphinxParser):
                 config = merge_file_level(config, topmatter, warning)
 
         parser = create_md_parser(config, SphinxRenderer)
+        # TODO: In the first pass, the call above will use MarkdownIt over the whole document,
+        #       populating the ``.md_env`` correctly. Over the next passes, from transforms as
+        #       ``sphinx.transforms.i18n.Locale`` it will create a blank new document, as well
+        #       as a new MarkdownIT parser but using just the Docutils document that is being
+        #       translated. As a result of this, the translation has no reference-links, and it
+        #       gives you the translation without resolving the links. Here we just re-use the
+        #       ``md_env`` dictionary that contains the ``.references`` populated in the first
+        #       pass, fixing i18n with MyST Parser.
+        env = {} if not hasattr(document.settings, 'md_env') else document.settings.md_env
         parser.options["document"] = document
-        parser.render(inputstring)
+        parser.render(inputstring, env)
+        document.settings.md_env = parser.renderer.md_env

I will share in their issue and see if they can come up with a better solution. Not sure whether we should use this patched version or start moving things to reST. On one hand this works but has the risk of getting out of sync with MyST (even though the diff is minimal), while on the other migrating everything to reST is a major effort.

Anyway, now at least we know what's wrong in MyST and I can stop thinking about this (interesting!) bug 🐛 🐞

image

Cheers
-Bruno

p.s.: I believe MyST will fix this issue soon-ish, as it's used in PyData and other Jupyter projects, but if there's haste in translating, at least we have a possible workaround....

@kinow
Copy link
Member Author

kinow commented Jan 26, 2023

(fixing the rtd build):

ModuleNotFoundError: No module named 'myst_parser.warnings_'

@kinow
Copy link
Member Author

kinow commented Jan 26, 2023

(had patched MyST's master branch 😬 changed to the 0.18.1 version we are using and applied to patch again 🤞 )

@kinow kinow requested a review from mr-c January 26, 2023 23:23
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Wow! Thank you for this @kinow !

I'm seeing lots of warnings that the default make html makes into errors (but readthedocs does not)

/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/expressions.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/file-formats.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/index.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/inputs.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/metadata-and-authorship.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/operations.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/outputs.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/parameter-references.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/requirements-and-hints.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/specifying-software-requirements.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/specifying-software-requirements.md:41: WARNING: Duplicate reference definition: SCICRUNCH [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/staging-input-files.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/staging-input-files.md:41: WARNING: Duplicate reference definition: SCICRUNCH [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/troubleshooting.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/troubleshooting.md:41: WARNING: Duplicate reference definition: SCICRUNCH [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/using-containers.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/using-containers.md:41: WARNING: Duplicate reference definition: SCICRUNCH [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/workflows.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/workflows.md:41: WARNING: Duplicate reference definition: SCICRUNCH [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/yaml-guide.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/topics/yaml-guide.md:41: WARNING: Duplicate reference definition: SCICRUNCH [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/tutorials.md:114: WARNING: Duplicate reference definition: FILE-PROP [myst.ref]
/home/docs/checkouts/readthedocs.org/user_builds/common-workflow-languageuser-guide/checkouts/370/src/tutorials.md:41: WARNING: Duplicate reference definition: SCICRUNCH [myst.ref]

https://readthedocs.org/projects/common-workflow-languageuser-guide/builds/19289547/

@mr-c
Copy link
Member

mr-c commented Jan 27, 2023

@kinow I added commits to revert the LICENSE .md to .rst conversion (this is separate from the warnings issue)

@kinow
Copy link
Member Author

kinow commented Jan 27, 2023

I'm seeing lots of warnings that the default make html makes into errors (but readthedocs does not)

Ah, without debugging or looking at the code, my guess is that it could be the code merging two md_env that contain the same link. AFAIK, we should be fine as I don't believe we will have the same alias for a different link (i.e. cc-license pointing to two different places in two different files). But I'll have to take a look in the code to confirm this, or see if it's something else. Thanks for testing, and feel free to update this PR (moving houses yet again 😅 little bandwidth today/tomorrow).

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Okay, we can fix the warnings later

@mr-c mr-c merged commit 3cf11ae into main Jan 27, 2023
@mr-c mr-c deleted the fix-i18n-myst branch January 27, 2023 13:06
@kinow kinow mentioned this pull request Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert Md to ReST to work around issue with translations

3 participants