-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add support for rowspan/colspan to tables #642
Conversation
277d8b0
to
451ad15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@utzig Changes so far look good to me, but I don't observe any changes on the table example page and make pdf
does throw an exception. Presumably still work in progress?
Not WIP, all the tables should render correctly with this PR, they do here, and I don't see issues with "make pdf" locally. Are you sure you tested with the PR's commits? |
@utzig commit 451ad15, Sphinx 3.0.2,
Tables render the same as https://breathe.readthedocs.io/en/latest/tables.html with What Sphinx version do you have locally? I am thinking my ancient version is to blame here. |
3.4.3 |
I tried with Sphinx==3.0.2 and it works fine as well, what docutils version do you have? |
@utzig docutils 0.16 (Bit short on time, will have more proper reply and testing hopefully soon.) |
Same problem here! I found out the problem, Doxygen>=1.9.0 is required because the previous versions didn't have |
@utzig Ah yes of course, my Doxygen is version 1.8.13 as I'm running Debian stable (buster).
I can't recall anything about this, is there some problem with this in master? |
I believe the issue is with my parser changes, or maybe rendering. |
This is related to the same problem you suggested in the previous PR to use |
The failures in the Sphinx latex writer happen on this line: https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/writers/latex.py#L963 Since |
@utzig Do you have some news about this? If not, I think we should merge this as-is keeping the |
Expand parser to support parsing extra "entry" parameters from recent Doxygen versions, namely "align", "rowspan" and "colspan". This should allow the renderer to correctly render tables once updated. Signed-off-by: Fabio Utzig <[email protected]>
Signed-off-by: Fabio Utzig <[email protected]>
"rowspan" is unable to span across different "thead", and "tbody" elements which make it not useful with the standard row parsing routines. This commit adds a final step when parsing a table to join all "thead" and "tbody" rows into just two set of elements. This still has the limitation that a rowspan from a header won't span to body elements, but this is not the most usual situation. Signed-off-by: Fabio Utzig <[email protected]>
To correct myself, the Latex writer does actually support spans, the problem with the code I pointed previously is that it expects that valid spans exist, and in that case all correct data structures are built in memory; which is not possible if using invalid Doxygen XML ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better descriptions where added to the commits themselves. After this is merged, the only remaining support that needs to be added to tables is
align
. It also fixes the issues previously seen with the latex writer.