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.

Sign up for GitHub

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

Jump to bottom

👌 Improve footnote def/ref warnings and translations #931

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

👌 Improve footnote def/ref warnings and translations #931

wants to merge 6 commits into from

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented May 13, 2024 •
edited

Add warnings (with source mapping) for unused footnote definitions and for footnote references that have no definition.
Also account for parsing translation snippets containing footnote references.

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

@chrisjsewell
👌 Improve footnote warnings
b226b53 
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 •
edited by codecov bot

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.18%. Comparing base (3d84ff8) to head (0961450).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
+ Coverage   90.13%   90.18%   +0.05%     
==========================================
  Files          24       24              
  Lines        3416     3425       +9     
==========================================
+ Hits         3079     3089      +10     
+ Misses        337      336       -1
Flag Coverage Δ
pytests 90.18% <100.00%> (+0.05%) ⬆️

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 •
edited

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

@chrisjsewell chrisjsewell mentioned this pull request May 13, 2024
Open
chrisjsewell
chrisjsewell commented May 13, 2024
View reviewed changes
myst_parser/mdit_to_docutils/base.py
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

chrisjsewell
chrisjsewell commented May 13, 2024
View reviewed changes
tests/test_renderers/fixtures/reporter_warnings.md
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 •
edited

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@chrisjsewell @codecov-commenter @asmeurer