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

src/sage_docbuild/conf.py: Do not fail if pplpy doc is not installed #35536

Merged
merged 3 commits into from
May 22, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 18, 2023

📚 Description

We change the sphinx configuration so that the object inventory of pplpy is only used when it is installed.

We currently have no way to install the pplpy documentation when using the all-conda installation method (#35528).

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

Co-authored-by: Isuru Fernando <isuruf@gmail.com>
@jhpalmieri
Copy link
Member

jhpalmieri commented May 3, 2023

I don't know if this is a valid review strategy: I did make pplpy_doc, then deleted local/share/doc/pplpy, then did make. The docbuild failed, saying

[reference] WARNING: failed to reach any of the inventories with the following issues:
[reference] intersphinx inventory 'https://www.labri.fr/perso/vdelecro/pplpy/latest/objects.inv' not fetchable due to <class 'AttributeError'>: No such config value: intersphinx_timeout

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 3, 2023

Thanks for testing! I've pushed a fix for this

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 3, 2023

With the same test method, I have verified that files such as local/share/doc/sage/html/en/reference/discrete_geometry/sage/geometry/cone.html contain the correct links (e.g. C_Polyhedron)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 3, 2023

Previous discussion:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 3, 2023

I guess we should also update src/doc/common/python3.inv; that's #35608

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 3, 2023

A better solution may be to use the configuration "Multiple targets for the inventory" in
https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html#confval-intersphinx_mapping

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 3, 2023

In fact, that's what the branch in #29231 does. I'll revive it

@github-actions
Copy link

github-actions bot commented May 3, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 4b7d3e3

@jhpalmieri
Copy link
Member

This works for me. (I used a different testing strategy: temporarily changed pplpy_doc to optional and removed it as a dependency from other packages.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 4, 2023

Thank you!

@mkoeppe mkoeppe modified the milestones: sage-10.0, sage-10.1 May 20, 2023
@vbraun vbraun merged commit 5a2cbb7 into sagemath:develop May 22, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 25, 2023
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
Math equations should be marked in the math environment. Change the
Sphinx markup in src/sage/categories/crystals.py
<!-- Why is this change required? What problem does it solve? -->
Fix some Sphinx markup as posted in Issue sagemath#35496
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
Fixes sagemath#35496
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [ ] The title is concise, informative, and self-explanatory.
- [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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- sagemath#35536: branch is on top of this PR to fix a build issue in conda

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35537
Reported by: xuluze
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 27, 2023
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
Math equations should be marked in the math environment. Change the
Sphinx markup in src/sage/categories/crystals.py
<!-- Why is this change required? What problem does it solve? -->
Fix some Sphinx markup as posted in Issue sagemath#35496
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
Fixes sagemath#35496
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [ ] The title is concise, informative, and self-explanatory.
- [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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- sagemath#35536: branch is on top of this PR to fix a build issue in conda

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35537
Reported by: xuluze
Reviewer(s):
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.

4 participants