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

👌 Improve footnote def/ref warnings and translations #931

Merged
merged 10 commits into from
Aug 5, 2024

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented May 13, 2024

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: sphinx-doc/sphinx#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

Add warnings (with source mapping) for unused footnote definitions and footnote references that have no definition.
@chrisjsewell
Copy link
Member Author

All good @asmeurer 😄 ?

@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

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 ⚠️
Additional details and impacted files
@@            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 Coverage Δ
pytests 90.30% <92.40%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chrisjsewell chrisjsewell changed the title 👌 Improve footnote definition/reference warnings 👌 Improve footnote definition/reference warnings and translation May 13, 2024
@chrisjsewell chrisjsewell changed the title 👌 Improve footnote definition/reference warnings and translation 👌 Improve footnote def/ref warnings and translations May 13, 2024
@asmeurer
Copy link
Contributor

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
Copy link
Member Author

chrisjsewell commented May 13, 2024

I tried this but I got an exception:

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

@asmeurer
Copy link
Contributor

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

@chrisjsewell
Copy link
Member Author

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 conda-forge/mdit-py-plugins-feedstock#21

@asmeurer
Copy link
Contributor

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:.

Comment on lines 306 to 310
# lets remove the footnote references, so that docutils does not produce any more warnings
# we need to replace them with an element though, so that ids can be moved over (otherwise docutils excepts)
if node.get("auto"):
self.document.autofootnote_refs.remove(node)
node.replace_self(nodes.inline(text=f"[^{node['refname']}]"))
Copy link
Member Author

Choose a reason for hiding this comment

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

code to suppress docutils warnings

Comment on lines 113 to 118
footnote reference with no definition
.
[^a]
.
<string>:1: (WARNING/2) No footnote definition found for label: 'a' [myst.footnote]
.
Copy link
Member Author

Choose a reason for hiding this comment

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

this produced additional docutils warnings, before the fix (https://github.com/executablebooks/MyST-Parser/pull/931/files#r1598103576)

@chrisjsewell
Copy link
Member Author

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
Copy link
Member Author

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

image

image

@asmeurer
Copy link
Contributor

asmeurer commented May 13, 2024

Sorry, don't have time to minimize it, but it's coming from Quansight-Labs/ndindex#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
Copy link
Contributor

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.

@chrisjsewell chrisjsewell merged commit 850f7c9 into master Aug 5, 2024
17 checks passed
@chrisjsewell chrisjsewell deleted the footnotes branch August 5, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants