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

Documentation - Upgrade Myst Parser #11194

Merged
merged 4 commits into from Jan 21, 2024

Conversation

NXPY123
Copy link
Contributor

@NXPY123 NXPY123 commented Nov 7, 2023

Resolves #11178. Myst Parser can be updated to 2.0.0, but support for Python 3.7 will be lost.

Please check the following:

  • Do the tests still pass?
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.

Copy link

squash-labs bot commented Nov 7, 2023

Manage this branch in Squash

Test this branch here: https://nxpy123update11178-update-myst-dzbh3.squash.io

setup.py Outdated Show resolved Hide resolved
@lb- lb- changed the title Upgrade Myst Parser to 0.19.0 Documentation - Upgrade Myst Parser Nov 7, 2023
@NXPY123 NXPY123 mentioned this pull request Nov 7, 2023
3 tasks
@lb- lb- added Documentation status:Needs Review dependencies Pull requests that update a dependency file labels Nov 7, 2023
@lb-
Copy link
Member

lb- commented Nov 8, 2023

When I run the docs build locally I get some loud warnings, all relating to the Django cross-reference, maybe there is a different config structure needed here.

My guess is that this would be a config issue https://myst-parser.readthedocs.io/en/latest/syntax/cross-referencing.html

Or maybe these warnings always existed and are now not suppressed https://myst-parser.readthedocs.io/en/latest/configuration.html#build-warnings

It looks like it's building correctly though.

Screenshot 2023-11-08 at 5 54 10 pm
writing output... [100%] topics/writing_templates
/code/wagtail/docs/advanced_topics/add_to_django_project.md:3: WARNING: 'myst' cross-reference target not found: 'django:intro/tutorial01' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/add_to_django_project.md:21: WARNING: 'myst' cross-reference target not found: 'django:topics/settings' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/add_to_django_project.md:21: WARNING: 'myst' cross-reference target not found: 'django:topics/http/urls' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/customisation/streamfield_blocks.md:258: WARNING: 'myst' cross-reference target not found: 'django:custom-deconstruct-method' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/deploying.md:7: WARNING: 'myst' cross-reference target not found: 'django:howto/deployment/index' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/deploying.md:17: WARNING: 'myst' cross-reference target not found: 'django:howto/deployment/wsgi/index' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/deploying.md:17: WARNING: 'myst' cross-reference target not found: 'django:howto/deployment/asgi/index' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/deploying.md:21: WARNING: 'myst' cross-reference target not found: 'django:howto/static-files/deployment' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/deploying.md:29: WARNING: 'myst' cross-reference target not found: 'django:topics/files' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/deploying.md:61: WARNING: 'myst' cross-reference target not found: 'django:topics/cache/index' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/deploying.md:71: WARNING: 'myst' cross-reference target not found: 'django:howto/deployment/checklist' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/i18n.md:28: WARNING: 'myst' cross-reference target not found: 'django:topics/i18n/translation' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/performance.md:136: WARNING: 'myst' cross-reference target not found: 'django:topics/performance' [myst.xref_missing]
/code/wagtail/docs/advanced_topics/testing.md:221: WARNING: 'myst' cross-reference target not found: 'django:howto/initial-data' [myst.xref_missing]
/code/wagtail/docs/contributing/developing.md:121: WARNING: 'myst' cross-reference target not found: 'django:ref/databases' [myst.xref_missing]
/code/wagtail/docs/contributing/developing.md:136: WARNING: 'myst' cross-reference target not found: 'django:ref/databases' [myst.xref_missing]
/code/wagtail/docs/contributing/first_contribution_guide.md:35: WARNING: 'myst' cross-reference target not found: 'django:intro/overview' [myst.xref_missing]
/code/wagtail/docs/contributing/first_contribution_guide.md:322: WARNING: 'myst' cross-reference target not found: 'django:topics/testing/overview' [myst.xref_missing]
/code/wagtail/docs/contributing/translations.md:13: WARNING: 'myst' cross-reference target not found: 'django:topics/i18n/translation' [myst.xref_missing]
/code/wagtail/docs/contributing/translations.md:41: WARNING: 'myst' cross-reference target not found: 'django:topics/i18n/translation' [myst.xref_missing]
/code/wagtail/docs/contributing/translations.md:107: WARNING: 'myst' cross-reference target not found: 'django:topics/i18n/translation' [myst.xref_missing]
/code/wagtail/docs/contributing/translations.md:110: WARNING: 'myst' cross-reference target not found: 'django:topics/i18n/translation' [myst.xref_missing]
/code/wagtail/docs/contributing/ui_guidelines.md:5: WARNING: 'myst' cross-reference target not found: 'django:ref/templates/language' [myst.xref_missing]
/code/wagtail/docs/extending/extending_client_side.md:28: WARNING: more than one target found for 'myst' cross-reference template_components: could be :std:ref:`template components` or :std:doc:`template components` [myst.xref_ambiguous]
/code/wagtail/docs/extending/extending_draftail.md:137: WARNING: 'myst' cross-reference target not found: '../topics/streamfield.rst' [myst.xref_missing]
/code/wagtail/docs/extending/extending_draftail.md:197: WARNING: 'myst' cross-reference target not found: 'django:topics/forms/media' [myst.xref_missing]
/code/wagtail/docs/extending/forms.md:3: WARNING: 'myst' cross-reference target not found: 'django:topics/forms/index' [myst.xref_missing]
/code/wagtail/docs/extending/rich_text_internals.md:18: WARNING: more than one target found for 'myst' cross-reference rich_text: could be :std:ref:`RichTextField` or :py:func:`wagtail.test.utils.form_data.rich_text` [myst.xref_ambiguous]
/code/wagtail/docs/extending/rich_text_internals.md:18: WARNING: 'myst' cross-reference target not found: '../topics/streamfield.rst' [myst.xref_missing]
/code/wagtail/docs/getting_started/integrating_into_django.md:108: WARNING: 'myst' cross-reference target not found: 'django:howto/static-files/deployment' [myst.xref_missing]
/code/wagtail/docs/getting_started/tutorial.md:228: WARNING: 'myst' cross-reference target not found: 'django:ref/templates/builtins' [myst.xref_missing]
/code/wagtail/docs/reference/contrib/forms/index.md:8: WARNING: 'myst' cross-reference target not found: 'django:topics/forms/index' [myst.xref_missing]
/code/wagtail/docs/reference/contrib/routablepage.md:33: WARNING: 'myst' cross-reference target not found: 'django:topics/http/urls' [myst.xref_missing]
/code/wagtail/docs/reference/hooks.md:8: WARNING: 'myst' cross-reference target not found: 'django:topics/signals' [myst.xref_missing]
/code/wagtail/docs/reference/hooks.md:273: WARNING: 'myst' cross-reference target not found: 'django:topics/http/urls' [myst.xref_missing]
/code/wagtail/docs/reference/pages/panels.md:314: WARNING: 'myst' cross-reference target not found: 'django:ref/models/fields' [myst.xref_missing]
/code/wagtail/docs/reference/pages/panels.md:356: WARNING: 'myst' cross-reference target not found: 'django:ref/forms/widgets' [myst.xref_missing]
/code/wagtail/docs/reference/settings.md:3: WARNING: 'myst' cross-reference target not found: 'django:ref/settings' [myst.xref_missing]
/code/wagtail/docs/reference/signals.md:3: WARNING: 'myst' cross-reference target not found: 'django:topics/signals' [myst.xref_missing]
/code/wagtail/docs/reference/signals.md:63: WARNING: 'myst' cross-reference target not found: 'django:topics/signals' [myst.xref_missing]
/code/wagtail/docs/releases/3.0.md:200: WARNING: 'myst' cross-reference target not found: 'django:ref/forms/widgets' [myst.xref_missing]
/code/wagtail/docs/releases/5.2.md:239: WARNING: 'myst' cross-reference target not found: 'wagtail_moderation_enabled' [myst.xref_missing]
/code/wagtail/docs/releases/5.2.md:357: WARNING: 'myst' cross-reference target not found: 'django:topics/forms/media' [myst.xref_missing]
/code/wagtail/docs/releases/5.2.md:502: WARNING: Lexing literal_block '{% load wagtailadmin_tags %}\n<script type="text/django-form-template" id="id_{{ formset.prefix }}-EMPTY_FORM_TEMPLATE">\n    {% escapescript %}\n        <div>Widget template content</div>\n        <script src="/js/my-widget.js"></script>\n    {% endescapescript %}\n</script>\n' as "html+django" resulted in an error at token: '/'. Retrying in relaxed mode.
/code/wagtail/docs/topics/images.md:389: WARNING: more than one target found for 'myst' cross-reference rich_text: could be :std:ref:`Rich Text (HTML)` or :py:func:`wagtail.test.utils.form_data.rich_text` [myst.xref_ambiguous]
/code/wagtail/docs/topics/pages.md:5: WARNING: 'myst' cross-reference target not found: 'django:ref/models/fields' [myst.xref_missing]
/code/wagtail/docs/topics/search/searching.md:9: WARNING: 'myst' cross-reference target not found: 'django:ref/models/querysets' [myst.xref_missing]
/code/wagtail/docs/topics/search/searching.md:263: WARNING: 'myst' cross-reference target not found: 'django:django.http.QueryDict' [myst.xref_missing]
/code/wagtail/docs/topics/snippets/rendering.md:9: WARNING: 'myst' cross-reference target not found: 'django:howto/custom-template-tags' [myst.xref_missing]
/code/wagtail/docs/topics/writing_templates.md:5: WARNING: 'myst' cross-reference target not found: 'django:topics/templates' [myst.xref_missing]
/code/wagtail/docs/topics/writing_templates.md:8: WARNING: 'myst' cross-reference target not found: 'django:ref/templates/api' [myst.xref_missing]
/code/wagtail/docs/topics/writing_templates.md:32: WARNING: 'myst' cross-reference target not found: 'django:ref/templates/api' [myst.xref_missing]
/code/wagtail/docs/topics/writing_templates.md:75: WARNING: 'myst' cross-reference target not found: 'django:howto/custom-template-tags' [myst.xref_missing]
generating indices... py-modindex done
writing additional pages... search done
copying images... [100%] _static/images/image_filter_fill_focal_close.png
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 53 warnings.

The HTML pages are in _build/html.

Build finished. The HTML pages are in _build/html.
root@2b1e1f152eee:/code/wagtail/docs# 

@NXPY123
Copy link
Contributor Author

NXPY123 commented Nov 9, 2023

I think it might be because the pages no longer exist in the Django site?

@lb-
Copy link
Member

lb- commented Nov 9, 2023

I do not think that's it, for example Django topics signals exists https://docs.djangoproject.com/en/4.1/topics/signals/

My guess is that MyST is expecting to read it's own config for these Django ones but the parsing / processing of those links happens in RTD (Read The Docs).

see docs/conf.py (docs - https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html)

# sphinx.ext.intersphinx settings
intersphinx_mapping = {
    "django": (
        "https://docs.djangoproject.com/en/stable/",
        "https://docs.djangoproject.com/en/stable/_objects/",
    )
}

Sorry I do not know much more about how this works but hopefully that helps you debug.

Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

You might have to update those links to use the inv: syntax and possibly update the configuration as well:
https://myst-parser.readthedocs.io/en/latest/syntax/cross-referencing.html#cross-project-intersphinx-links

@lb-
Copy link
Member

lb- commented Nov 11, 2023

I usually find that I have to run make clean and then make html to get all the errors to correctly show. Not sure if that will change with the new update but I guess that it only does incremental builds and then only checks 'changed files' when running make clean a second time.
Not sure if that helps though.

Thanks for keeping at this.

@NXPY123
Copy link
Contributor Author

NXPY123 commented Nov 11, 2023

Thanks for that info! The warnings come up when I do make clean and make html. There are warnings to references to a bunch of other sites too (Twitter,Github,Draftail etc). I'll try applying the suggestions you guys gave.

@NXPY123
Copy link
Contributor Author

NXPY123 commented Nov 11, 2023

Some of the warnings are because there's a # infront of the URL in the html built by Sphinx. For example, this warning:

/code/wagtail/docs/releases/5.0.md:24: WARNING: 'myst' cross-reference target not found: 'https://yougov.com/' [myst.xref_missing]

This is the line that gives the warning in the html file built by sphinx:

This feature was developed by Joshua Munn, and sponsored by <a class="reference internal" href="#https://yougov.com/"><span class="xref myst">YouGov</span></a>.</p>

The URL works when you remove the # in front of it.

Even for django, it only places a # infront of the url it generates instead of considering it as an intersphinx mapping:

<p>The simplest way to make your snippets available to templates is with a template tag. This is mostly done with vanilla Django, so perhaps reviewing Django’s documentation for <a class="reference internal" href="#django:howto/custom-template-tags"><span class="xref myst">custom template tags</span></a> will be more helpful. We’ll go over the basics, though, and point out any considerations to make for Wagtail.</p>

@NXPY123
Copy link
Contributor Author

NXPY123 commented Nov 11, 2023

External links require http and https to be specified in myst_url_schemes:


myst_url_schemes = {
    "https" : None,
    "http" : None,
    "guide": "https://guide.wagtail.org/en-latest/{{path}}#{{fragment}}"
}

@lb-
Copy link
Member

lb- commented Nov 11, 2023

Great digging @NXPY123 that seems like it could be a path in the right direction. We would want to probably add mailto:.

Let's not add the guide in this PR though, that can be added in a future PR related to that issue.

@NXPY123
Copy link
Contributor Author

NXPY123 commented Nov 27, 2023

Based on the docs:

image

None of the URI's aren't prefixed by inv and also I'm not sure if the key,domain,type and name are according to the inventory file. That might be the issue.

@NXPY123
Copy link
Contributor Author

NXPY123 commented Dec 5, 2023

@lb-

The Django inv file is in this format:

py:attribute
    django.apps.AppConfig.default                                                    : ref/applications/#django.apps.AppConfig.default
    django.apps.AppConfig.default_auto_field                                         : ref/applications/#django.apps.AppConfig.default_auto_field
    django.apps.AppConfig.label                                                      : ref/applications/#django.apps.AppConfig.label
    django.apps.AppConfig.models_module                                              : ref/applications/#django.apps.AppConfig.models_module
    django.apps.AppConfig.module                                                     : ref/applications/#django.apps.AppConfig.module
    django.apps.AppConfig.name                                                       : ref/applications/#django.apps.AppConfig.name
    django.apps.AppConfig.path                                                       : ref/applications/#django.apps.AppConfig.path
    django.apps.AppConfig.verbose_name                                               : ref/applications/#django.apps.AppConfig.verbose_name
    django.apps.apps.ready                                                           : ref/applications/#django.apps.apps.ready
    django.conf.settings.configured                                                  : topics/settings/#django.conf.settings.configured
    django.contrib.admin.AdminSite.app_index_template                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.app_index_template
    django.contrib.admin.AdminSite.empty_value_display                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.empty_value_display
    django.contrib.admin.AdminSite.enable_nav_sidebar                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.enable_nav_sidebar
    django.contrib.admin.AdminSite.final_catch_all_view                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.final_catch_all_view
    django.contrib.admin.AdminSite.index_template                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.index_template
    django.contrib.admin.AdminSite.index_title                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.index_title
    django.contrib.admin.AdminSite.login_form                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.login_form
    django.contrib.admin.AdminSite.login_template                                         : ref/contrib/admin/#django.contrib.admin.AdminSite.login_temp

So to generate url to ref/applications/#django.apps.AppConfig.default, the following format should be used:

[Text](inv:#django.apps.AppConfig.default)

@lb-
Copy link
Member

lb- commented Dec 5, 2023

@NXPY123 nice find. Can you maybe update one or two pages on this PR with that format. We can do some testing.

If that works and others on the core team are happy with that global change, I think we go for it.

Also. Maybe do a search around GitHub and see if any other documentation sites using MyST have that. Just a reference that we're not doing something wrong here.

@lb-
Copy link
Member

lb- commented Dec 5, 2023

@NXPY123 also please test a file with an RST embedded code, so we can check it works in that also.

@NXPY123
Copy link
Contributor Author

NXPY123 commented Dec 6, 2023

@NXPY123 also please test a file with an RST embedded code, so we can check it works in that also.

The following lines:

* {ref}`Creating models <django:creating-models>`
* {doc}`Model syntax <django:topics/db/models>`

are being rendered as:

<li><p><a class="reference external" href="https://docs.djangoproject.com/en/4.2/intro/tutorial02/#creating-models" title="(in Django v4.2)"><span class="xref std std-ref">Creating models</span></a></p></li>
<li><p><a class="reference external" href="https://docs.djangoproject.com/en/4.2/topics/db/models/" title="(in Django v4.2)"><span class="xref std std-doc">Model syntax</span></a></p></li>

in the html file

@NXPY123
Copy link
Contributor Author

NXPY123 commented Dec 6, 2023

In numpy docs the following lines are present:

Convert from a pandas DataFrame to a NumPy array
See :meth:`pandas.DataFrame.to_numpy`

In https://pandas.pydata.org/pandas-docs/stable/objects.inv, pandas.DataFrame.to_numpy points to reference/api/pandas.DataFrame.to_numpy.html#pandas.DataFrame.to_numpy

@lb- lb- self-requested a review December 6, 2023 22:21
Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

@NXPY123 thank you! This is looking great.

I think we need to make a few more tweaks but you have the go ahead to update all refs (based on the notes below).

  1. Keep the intersphinx mapping to the stable urls, not the 4.2 urls.
  2. After my 3rd reading of the docs section on intersphinx mapping I think we need to make one more tweak to the URL refs. https://myst-parser.readthedocs.io/en/latest/syntax/cross-referencing.html#cross-project-intersphinx-links (see code below)

Instead of (django:ref/models/fields) we should be using the (inv:django#ref/models/fields) note the usage of inv:django. This says use the inv schema (as in a custom URL resolover) and then use the key 'django'.

Finally the #name format means that the name it will search for will be ref/models/fields in this case.

From the docs...

you can then reference inventory objects by prefixing the inv schema to the destination URI: inv:key:domain:type#name.

key, domain and type are optional, e.g. for inv:#name, all inventories, domains and types will be searched, with a warning emitted if multiple matches are found.

Additionally, * is a wildcard which matches zero or characters, e.g. inv::std:doc#a will match all std:doc objects in all inventories, with a name beginning with a. Note, to match to a literal * use *.

@lb-
Copy link
Member

lb- commented Jan 20, 2024

@NXPY123 I have an update coming to this branch with the reference changes as this ended up being pretty close.

I will also push an update to the documentation guidelines with a basic example of these new formats.

Unfortunately it appears that there may be differences in the rst vs markdown format but most usage is MD format so it should be ok.

@lb- lb- force-pushed the update/11178-update-myst-parser branch from 142f262 to fca7210 Compare January 20, 2024 23:41
@lb- lb- force-pushed the update/11178-update-myst-parser branch from fca7210 to 9c6f582 Compare January 20, 2024 23:56
Copy link
Member

@lb- lb- 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 @NXPY123 this is a great win!

Hopefully MyST will add some improvements to the way intersphinx refs work but overall it's great to have this updated and aligned with the latest conventions.

Thanks also for pushing through and learning how to use this docs setup really well.

@lb- lb- merged commit 24725fe into wagtail:main Jan 21, 2024
17 checks passed
docs/advanced_topics/third_party_tutorials.md Show resolved Hide resolved
docs/contributing/developing.md Show resolved Hide resolved
docs/contributing/documentation_guidelines.md Show resolved Hide resolved
docs/topics/pages.md Show resolved Hide resolved
lb- added a commit to lb-/wagtail that referenced this pull request Jan 21, 2024
Partially reverts commit 7fa335c (fixing some issues with the URL updates)
lb- added a commit that referenced this pull request Jan 21, 2024
Partially reverts commit 7fa335c (fixing some issues with the URL updates)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation dependency - Upgrade Myst Parser
3 participants