bpo-37504: Fix documentation build with texinfo builder #14606
Conversation
There was a problem hiding this comment.
@mitya57 Thanks for the contribution! In order to fully verify that the change is working appropriately since this is addressing a specific bug (Documentation fails to build when using Sphinx' textinfo builder), I would advise advise updating the tests (which would be included in the PR). A screenshot does provide a solid visual example, but is usually not reliable enough to show that it fully functions as described in the initial issue. This also helps to ensure that if your addition works as intended, no other changes will unintentionally break it.
The above suggestions might not be necessary in order for your PR to be merged, but in general adding tests is always helpful, especially when it involves a change in functionality or fixing a bug that was under the radar of the existing tests.
|
Now the texinfo file is built successfully during Travis test, see https://travis-ci.org/python/cpython/jobs/555099128#L283. |
|
@zooba @tirkarthi As you were the author and reviewer of #14406 which added this The Travis build failed with a temporary error, I guess it can be retried. As your original PR is not part of any stable release, I think the “skip news” label can be added. |
@mitya57 You can retry the travis build by adding and removing an empty comment
Agreed, I think the |
There was a problem hiding this comment.
Thanks, I didn't know about texinfo and reading it on Emacs is nice. I am okay with the changes as per the reference and running locally also builds fine. I am not sure about running make texinfo on primary CI like Travis. I would defer to @JulienPalard on that.
It was not my idea, but requested by @aeros167. |
From what I recall, that suggestion was just to add further testing, I'm not certain as to the best way to do it with regards to texinfo or if it should even be done through travis. Julien would definitely be far better to ask, also @willingc since she's a documentation expert. |
|
Thanks @mitya57 for the pull request. A few thoughts on the changes:
|
In the table model used by docutils, the `cols` attribute of `tgroup` nodes is mandatory, see [1]. It is used in texinfo builder in [2]. [1]: https://www.oasis-open.org/specs/tm9901.htm#AEN348 [2]: https://github.com/sphinx-doc/sphinx/blob/v2.1.2/sphinx/writers/texinfo.py#L1129
|
I have now rebased on latest master and updated as suggested by @willingc. |
In the table model used by docutils, the `cols` attribute of `tgroup` nodes is mandatory, see [1]. It is used in texinfo builder in [2]. [1]: https://www.oasis-open.org/specs/tm9901.htmGH-AEN348 [2]: https://github.com/sphinx-doc/sphinx/blob/v2.1.2/sphinx/writers/texinfo.pyGH-L1129 * Doc: Add texinfo support to the Makefile (cherry picked from commit c3d679f) Co-authored-by: Dmitry Shachnev <mitya57@users.noreply.github.com>
|
GH-15866 is a backport of this pull request to the 3.8 branch. |
In the table model used by docutils, the `cols` attribute of `tgroup` nodes is mandatory, see [1]. It is used in texinfo builder in [2]. [1]: https://www.oasis-open.org/specs/tm9901.htmGH-AEN348 [2]: https://github.com/sphinx-doc/sphinx/blob/v2.1.2/sphinx/writers/texinfo.pyGH-L1129 * Doc: Add texinfo support to the Makefile (cherry picked from commit c3d679f) Co-authored-by: Dmitry Shachnev <mitya57@users.noreply.github.com>
In the table model used by docutils, the `cols` attribute of `tgroup` nodes is mandatory, see [1]. It is used in texinfo builder in [2]. [1]: https://www.oasis-open.org/specs/tm9901.htm#AEN348 [2]: https://github.com/sphinx-doc/sphinx/blob/v2.1.2/sphinx/writers/texinfo.py#L1129 * Doc: Add texinfo support to the Makefile
In the table model used by docutils, the
colsattribute oftgroupnodes is mandatory. It is used in texinfo builder here.Here is a screenshot proving that this change works:
https://bugs.python.org/issue37504