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

Remove nitpick_patch_config function #38100

Merged
merged 4 commits into from
Jun 1, 2024

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented May 27, 2024

Fixes #37996.

We remove this function

def nitpick_patch_config(app):
    """
    Patch the default config for nitpicky
    [...] ensure that nitpicky is not considered as a Sphinx
    environment variable but rather as a Sage environment variable. As a
    consequence, changing it doesn't force the recompilation of the entire
    documentation.
    """
    app.config.values['nitpicky'] = (False, 'sage')
    app.config.values['nitpick_ignore'] = ([], 'sage')

from src/sage_docbuild/conf.py.

The original purpose of the function seems to change "env" to "sage" for nitpicky config variable. But the recent Sphinx is setting "" (empty string) instead of "env" by default. See https://github.com/sphinx-doc/sphinx/blob/48cbb43e28efe82b198137042811e0ade3599ae7/sphinx/config.py#L250C1-L250C41

  • Hence the function is already no-op. But this old and obsolete function is the cause of sage doc does not even start to build with sphinx 7.3.7 #37996. Hence we simply remove it.

  • To prepare for Sphinx 7.3.7, we also remove some code which seems to have become obsolete when sage -clone was removed long time ago.

  • Made some docstring up-to-date, to test incremental doc build.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented May 27, 2024

Documentation preview for this PR (built with commit 6489d4c; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@collares
Copy link
Contributor

I've tested a non-incremental build with Sphinx 7.3.7 and it looks good. Thanks!

@kwankyu
Copy link
Collaborator Author

kwankyu commented May 28, 2024

Thanks. Then let us ship this PR.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 29, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37996.

We remove this function

```python
def nitpick_patch_config(app):
    """
    Patch the default config for nitpicky
    [...] ensure that nitpicky is not considered as a Sphinx
    environment variable but rather as a Sage environment variable. As a
    consequence, changing it doesn't force the recompilation of the
entire
    documentation.
    """
    app.config.values['nitpicky'] = (False, 'sage')
    app.config.values['nitpick_ignore'] = ([], 'sage')
```
from `src/sage_docbuild/conf.py`.

The original purpose of the function seems to change `"env"` to `"sage"`
for `nitpicky` config variable. But the recent Sphinx is setting `""`
(empty string) instead of `"env"` by default. See
https://github.com/sphinx-doc/sphinx/blob/48cbb43e28efe82b198137042811e0
ade3599ae7/sphinx/config.py#L250C1-L250C41

- Hence  the function is already no-op. But this old and obsolete
function is the cause of sagemath#37996. Hence we simply remove it.

- To prepare for Sphinx 7.3.7, we also remove some code which seems to
have become obsolete when `sage -clone` was removed long time ago.

- Made some docstring up-to-date, to test incremental doc build.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38100
Reported by: Kwankyu Lee
Reviewer(s):
@vbraun vbraun merged commit e1b2269 into sagemath:develop Jun 1, 2024
15 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sage doc does not even start to build with sphinx 7.3.7
4 participants