👌 Improve footnote def/ref warnings and translations

chrisjsewell commented 4 months ago

Footnotes are now parsed similar to the corresponding restructuredtext, in that resolution (between definitions and references) and ordering is now deferred to transforms on the doctree.

This allows for the proper interaction with other docutils/sphinx transforms, including those that perform translations.

In addition, an upstream improvement to unreferenced footnote definitions is also added here: https://github.com/sphinx-doc/sphinx/pull/12730, so that unreferenced and duplicate definitions are correctly warned about, e.g.:

source/index.md:1: WARNING: Footnote [1] is not referenced. [ref.footnote]
source/index.md:4: WARNING: Duplicate footnote definition found for label: 'a' [ref.footnote]

It is of note that warnings for references with no corresponding definitions are deferred to docutils to handle, e.g. for [^a] with no definition:

source/index.md:1: ERROR: Too many autonumbered footnote references: only 0 corresponding footnotes available. [docutils]
source/index.md:1: ERROR: Unknown target name: "a". [docutils]

These warning messages are a little obscure, and it would be ideal that one clear warning was emitted for the issue. However, it is non-trivial in this extension; to both suppress the existing warnings, and then replace them with a better one, so for now we don't do it here, and ideally this would be improved upstream in docutils.

closes #930 closes #884 closes #801 closes #445

chrisjsewell commented 4 months ago

All good @asmeurer 😄 ?

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 92.40506% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.30%. Comparing base (401e08c) to head (9041bbd).

Files	Patch %	Lines
myst_parser/mdit_to_docutils/transforms.py	86.95%	6 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #931 +/- ## ========================================== - Coverage 90.34% 90.30% -0.04% ========================================== Files 24 24 Lines 3469 3507 +38 ========================================== + Hits 3134 3167 +33 - Misses 335 340 +5 ``` | [Flag](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/931/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | Coverage Δ | | |---|---|---| | [pytests](https://app.codecov.io/gh/executablebooks/MyST-Parser/pull/931/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | `90.30% <92.40%> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

asmeurer commented 4 months ago

I tried this but I got an exception:

Traceback (most recent call last):
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/cmd/build.py", line 298, in build_main
    app.build(args.force_all, args.filenames)
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/application.py", line 355, in build
    self.builder.build_update()
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 293, in build_update
    self.build(to_build,
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 313, in build
    updated_docnames = set(self.read())
                           ^^^^^^^^^^^
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 420, in read
    self._read_serial(docnames)
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 441, in _read_serial
    self.read_doc(docname)
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/builders/__init__.py", line 498, in read_doc
    publisher.publish()
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/docutils/core.py", line 234, in publish
    self.document = self.reader.read(self.source, self.parser,
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/sphinx/io.py", line 105, in read
    self.parse()
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/docutils/readers/__init__.py", line 76, in parse
    self.parser.parse(self.input, document)
  File "/Users/aaronmeurer/Documents/MyST-Parser/myst_parser/parsers/sphinx_.py", line 73, in parse
    parser = create_md_parser(config, SphinxRenderer)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aaronmeurer/Documents/MyST-Parser/myst_parser/parsers/mdit.py", line 64, in create_md_parser
    .use(footnote_plugin, inline=False, move_to_end=False, always_match_refs=True)
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aaronmeurer/miniconda3/envs/ndindex-matplotlib-dev/lib/python3.12/site-packages/markdown_it/main.py", line 253, in use
    plugin(self, *params, **options)
TypeError: footnote_plugin() got an unexpected keyword argument 'inline'

chrisjsewell commented 4 months ago

I tried this but I got an exception:

you haven't updated your mdit-py-plugins version

asmeurer commented 4 months ago

I have the latest version from conda. Is there a dev version I need to use?

chrisjsewell commented 4 months ago

I have the latest version from conda. Is there a dev version I need to use?

yeh it was only released on pypi today, so this has only just appeared https://github.com/conda-forge/mdit-py-plugins-feedstock/pull/21

asmeurer commented 4 months ago

OK, I was able to get it working. Two things I noticed:

The line number for WARNING: No footnote definition found for label: is wrong. It is giving the line for the beginning of the paragraph instead of the line with the label.
The error message ERROR: Too many autonumbered footnote references: only 0 corresponding footnotes available. is confusing. I'm not sure I understand what it's trying to say. And it seems to be a duplicate error for the same line that gives ERROR: Unknown target name:.

chrisjsewell commented 4 months ago

It is giving the line for the beginning of the paragraph instead of the line with the label.

ah well, if you haven't noticed this the same for every "inline" warning in sphinx/docutils (for both restructuredtext and myst)

The error message ERROR: Too many autonumbered footnote references: only 0 corresponding footnotes available. is confusing, ...

these are all from docutils, not myst but thats strange, because there is specific code to suppress these (see https://github.com/executablebooks/MyST-Parser/pull/931/files#r1598103576) and they don't show up in the tests anymore (https://github.com/executablebooks/MyST-Parser/pull/931/files#r1598109555)

do you have a minimal working example, that could get the test to produce these errors

chrisjsewell commented 4 months ago

I definitely get "good" warnings now, if I add some test examples to the top of docs/index.md

asmeurer commented 4 months ago

Sorry, don't have time to minimize it, but it's coming from https://github.com/Quansight-Labs/ndindex/pull/136, commit 09541959137694579060a6c48b3e429fa8e30026

/Users/aaronmeurer/Documents/ndindex/docs/indexing-guide/multidimensional-indices/ellipses.md:101: WARNING: No footnote definition found for label: 'tuple-ellipsis-footnote' [myst.footnote]
/Users/aaronmeurer/Documents/ndindex/docs/indexing-guide/multidimensional-indices/tuples.md:403: ERROR: Too many autonumbered footnote references: only 0 corresponding footnotes available.
/Users/aaronmeurer/Documents/ndindex/docs/indexing-guide/multidimensional-indices/tuples.md:403: ERROR: Unknown target name: "integer-scalar-footnote".

asmeurer commented 4 months ago

It's possible it's coming from the other cross-reference in the same paragraph. I had to use a workaround to cross-reference cross-page footnotes. These errors all came from me splitting a document into multiple pages and forgetting to move the footnotes.

executablebooks / MyST-Parser

👌 Improve footnote def/ref warnings and translations #931

Codecov Report