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

Breaking API change for nitpick_ignore #11355

Closed
ksunden opened this issue Apr 24, 2023 · 6 comments
Closed

Breaking API change for nitpick_ignore #11355

ksunden opened this issue Apr 24, 2023 · 6 comments

Comments

@ksunden
Copy link
Contributor

ksunden commented Apr 24, 2023

Describe the bug

The change from a list to a set is a breaking API change which affects Matplotlib's sphinx build. We have an extension which expects this value to be a list (with the extend method thereof)

How to Reproduce

conf.py:

nitpicky=True
extensions = ['sphinxext.missing_references']

index.rst:

:class:`missing`

missing_references.json:

{
  "py:class": {
    "missing": [
      "doc/index.rst:1"
    ]
  }
}

sphinxext/missing_references.py:
Copied from matplotlib

Errors because it is expecting app.conf.nitpick_ignore to be a list, not a set.

Environment Information

Platform:              linux; (Linux-6.2.0-76060200-generic-x86_64-with-glibc2.35)
Python version:        3.11.0 (main, Oct 27 2022, 01:56:38) [GCC 11.3.0])
Python implementation: CPython
Sphinx version:        6.2.0
Docutils version:      0.19
Jinja2 version:        3.1.2
Pygments version:      2.14.0

Sphinx extensions

["sphinxext.missing_references"] 

Note this is the extension linked above from matplotlib, but the key is it is an extension which modifies `app.conf.nitpick_ignore`.

Additional context

Link to failing mpl doc build: https://app.circleci.com/pipelines/github/matplotlib/matplotlib/23801/workflows/28a233c0-3582-42a8-99fc-164922a2fe7a/jobs/75765

The line from sphinx:

'nitpick_ignore': (set(), None, [set, list, tuple]),

The commit where it changed:

7ecf037

@AA-Turner
Copy link
Member

Could you change app.config.nitpick_ignore.extend(ignored_references.keys()) to app.config.nitpick_ignore = set(app.config.nitpick_ignore).extend(ignored_references.keys())?

A

@tacaswell
Copy link
Contributor

We can definitely update our code, but that won't help anyone who has currently released versions of Matplotlib and try to build the docs with the latest version of sphinx.

@ksunden
Copy link
Contributor Author

ksunden commented Apr 24, 2023

To be clear, even if you do wish to allow set and tuple, changing the default back to list would at least mitigate effects felt by our (and any similiar) extensions. At that point, only new users using the new features would run into problems with extensions that cannot handle sets or tuples.

We can fix mpl's internal usage by even simply explicitly setting nitpick_ignore = [] in conf.py (or by updating the extension to handle any of those types), but we cannot retroactively set that for released versions of mpl.

@AA-Turner
Copy link
Member

The question from Sphinx's perspective is if the internal type of config.nitpick_ignore is considered to be public API, and if we bless modification by extensions.

I think reasonable to make list the default again though -- the only downside is that it's slower (we use membership tests for a given tuple that's about to be emitted, so sets are clearly the better choice).

A

@AA-Turner
Copy link
Member

I've released Sphinx 6.2.1 with a fix for this.

cc: @ksunden; @tacaswell

A

@tacaswell
Copy link
Contributor

Thank you @AA-Turner !

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants