common-workflow-language / user_guide

The CWL v1.0 - v1.2 user guide
http://www.commonwl.org/user_guide/
Other
42 stars 67 forks source link

Patch MyST to keep the context needed to translate Markdown files #370

Closed kinow closed 1 year ago

kinow commented 1 year ago

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 :bug: :lady_beetle:

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 commented 1 year ago

(fixing the rtd build):

ModuleNotFoundError: No module named 'myst_parser.warnings_'
kinow commented 1 year ago

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

mr-c commented 1 year ago

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

kinow commented 1 year ago

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 :sweat_smile: little bandwidth today/tomorrow).