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

Replacing signature nodes with section nodes for sections #137

Closed
wants to merge 4 commits into from
Closed

Replacing signature nodes with section nodes for sections #137

wants to merge 4 commits into from

Conversation

vitaut
Copy link
Contributor

@vitaut vitaut commented Oct 24, 2014

The proposed PR replaces signature nodes with section nodes for sections such as "Public Functions". Addresses Issue #136. Here's an example demonstrating the new rendering: http://cppformat.readthedocs.org/en/master/reference.html#public-func.

Unfortunately I couldn't find how to get the class name (or other context information) in SectionDefTypeSubRenderer.render so the ID generated here simply uses data_object.kind which is not unique. It would be better to have <class-name>:<kind> (e.g. fmt::BasicWriter:public-func) or something similar depending on section type. Michael, could you suggest how to do this?

@michaeljones
Copy link
Collaborator

Hey, thank you for yet another contribution :) I'd agree that the indentation is a bit excessive and that it would be nice to find a replacement. Thank you for doing the research on the approach.

I didn't know about using sections. Is there any risk that it interferes with the document structure at all? I was under the impression that sections were special things for chunking up the document: http://docutils.sourceforge.net/docs/ref/doctree.html#section

It might be confusing if 'Public Functions' is considered a section but the class name isn't. We came across the issue a little with doxygen markdown support allowing markdown titles in comments which we'd like to make headings but reStructuredText doesn't seem to support titles without making sections (or that was the conclusion that I think we reached, nothing with rst is terribly obvious to me.)

Thoughts? It does seem to work for you and I like the look of the results (thank you for sharing your demonstration.)

@vitaut
Copy link
Contributor Author

vitaut commented Oct 24, 2014

Good point. Sections can indeed interfere with the document structure, so it might be better to use some other docutil nodes like rubric or container. Do you have any ideas/preferences regarding what kind of node may be better suited for this?

@michaeljones
Copy link
Collaborator

I've not used either a rubric or a container so thank you for the links. The combination looks like it might suit this situation well. I guess the container is optional but it won't harm but put some kind of box around each block and its contents. Depending on the affect it has on the PDF output. Hopefully it doesn't attempt to indent anything.

Happy for you to make the changes or I could make some kind of time related promise and we'll see how well I do this time :)

@rweickelt
Copy link
Contributor

I'm glad that You raised this up and that You provided some code that uses sections. The docutils documentation is a little bit scarce about how to use their code properly. I played with it for some time, but became very frustrated and threw it away.

We came across something similar in issue #116 and I ended up converting doxygen headlines to emphasized text. However, I totally agree, that a class documentation should be a structured document and needs sections. Public/whatever functions should imho form sections. Classes itself should be sections or even document titles.

Why do You think, this is dangerous? Which advantages can we expect by using rubrics/containers apart from not being indented?

@vitaut
Copy link
Contributor Author

vitaut commented Oct 25, 2014

@michaeljones Please do not do any promises, I noticed that without them you respond much faster =). Seriously though, thanks for the comments, I'll do some experiments with the new node types and try revising my implementation.

@rweickelt I think that whether classes should form sections or not depends on the documentation style, so, if supported, this would be better controlled by an option. As for the current issue, I'm not trying to revise the document structure by introducing sections, but only fix a slight misuse of the docutils markup. To be more specific, I'm trying to replace signature nodes used for parts other than signatures which results in extra indentation, with something more appropriate.

Replace section with compound and title with rubric. According to docutils. This preserves the document structure but gives the required rendering of section titles like "Public Functions".
@vitaut
Copy link
Contributor Author

vitaut commented Oct 25, 2014

OK, I've added one more commit that replaces section+title with compound+rubric. Here's how it looks like in cppformat docs:

HTML: http://cppformat.readthedocs.org/en/master/reference.html#write-api
PDF: https://media.readthedocs.org/pdf/cppformat/master/cppformat.pdf#page=8

The HTML looks slightly different, but it all depends on theme and can be adjusted by CSS. PDF looks very nice IMHO.

@michaeljones
Copy link
Collaborator

Much appreciated. Out of interest, why the compound node instead of the container? From the HTML writer it looks like they both end up as divs but compound adds some extra compound-first, compound-last and compound-middle css classes to its children. Probably happier with the container to be honest, though I haven't seen the visual results.

I'd agree that the PDF output looks good. We might try adding some custom styling to the HTML once we've sorted out that whole breathe.css file thingy :)

@vitaut
Copy link
Contributor Author

vitaut commented Oct 25, 2014

Ah, this was supposed to be a container, but my not fully awaken brain decided otherwise. Could you change it after merging provided that you are happy with the rest?

@michaeljones
Copy link
Collaborator

More than happy to make the change myself. Cheers for confirming that it should be a container.

@rweickelt, if you come up with a version based more around sections and strongly feel that it is appropriate, then I'd be happy to work on the integration and getting an option to switch between the modes. I'm hoping it is really just a case of managing the dictionary of Renderers that the RenderFactory is using (or whatever they are called :)

I can't quite decide myself if it is appropriate as I see sections as being for chapters & headings, or that sort of thing, but I realise there is some overlap.

@michaeljones
Copy link
Collaborator

Merged in the changes with s/compound/container. I've also added breathe-sectiondef and breathe-sectiondef-title css classes to the relevant nodes to allow us to experiment with formatting if desired.

Much appreciated as always!

@vitaut
Copy link
Contributor Author

vitaut commented Oct 27, 2014

Thanks, Michael!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants