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

Keep templates on adjacent lines #300

Merged

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented Jan 2, 2017

This is very visible on the ReadTheDocs theme, which draws two boxes, one around the template, and one around the rest of the signature:

image

Seems that sphinx 1.5 introduced a desc_signature_line that solves exactly this problem:

image

This is how using the cpp domain renders templates, so it would be good if breathe were consistent

This ought to be backwards compatible with 1.4, giving the same behaviour as before

@vitaut
Copy link
Contributor

vitaut commented Jan 8, 2017

Thanks a lot for the PR. Could you also add a test case to test_renderer.py?

@arximboldi
Copy link
Contributor

That's cool! I wonder if the same thing could be used to separate each template argument in a different line for very long template signatures with default arguments...

@eric-wieser
Copy link
Contributor Author

@vitaut: Not sure what to test for - are you content with simple text content? (ie, that this is not a regression from before from a text perspective?

@arximboldi: That doesn't seem within the scope of breathe, and is probably a feature request for sphinx

@vitaut
Copy link
Contributor

vitaut commented Jan 11, 2017

Ideally, showing that the output is rendered on separate lines as expected, but, if that's too hard, at least that the declaration including template <...> is rendered.

@jakobandersen
Copy link
Collaborator

Sorry for breaking Breathe (again). Each signature is always a desc_signature node which has 1 or more desc_signature_line nodes as children. The desc_signature_line nodes have the sphinx_cpp_tagname attribute.

Also, there is now an option on the Sphinx directives, :tparam-line-spec:, which causes the template parameters to be separated into multiple lines, as @arximboldi suggested. In the future the option may be given to specify the grouping of paramters on lines.

@eric-wieser
Copy link
Contributor Author

Guessing you're still waiting on a test here?

@svenevs
Copy link

svenevs commented Sep 29, 2017

@eric-wieser the test case for this can be really simple

/// Testing render signature of template struct
template <class T, class U>
struct template_struct { };

with

.. doxygenstruct:: template_struct

I'm trying to track down the other template issues, but the above is enough to test the render signature. But then again, the existing template (and specialization) tests should already be sufficient?

@eric-wieser
Copy link
Contributor Author

@svenevs: My issue was not knowing how to actually write the test - I wasn't able to find any clear example of what a breathe test looks like

@vermeeren vermeeren self-assigned this May 16, 2018
@vermeeren
Copy link
Collaborator

Templates are already built in examples/doxygen/templ.cpp.

As for the test_renderer.py test, currently there isn't one for templates. I think it is unfair to ask Eric, as a contributor, to write a new test when a small modification is applied to an existing component.

I took a look at the renderer tests to see if I could perhaps write one for templates. But indeed is isn't very clear. I'll see if I can manage somehow. If not, I intend to merge this anyway. I ran the changes on some personal projects and it looks a lot better with the RTD theme.

@svenevs
Copy link

svenevs commented May 16, 2018

@eric-wieser sorry for never responding, I didn't see your response.

@melvinvermeeren I don't believe breathe currently supports a test for this. In theory, the test could look something like this:

def test_template_struct_unspecialized():
    # template <class T, class U> struct unspecialized_struct { };
    member_def = TestMemberDef(
        kind='struct', name='unspecialized_struct',
        templateparamlist=[TestParam(type_='class T'), TestParam(type_='class U')]
    )
    signature = find_node(render(member_def), 'desc_signature')
    # print(signature.astext())
    assert signature.astext() == 'template <class T, class U> struct unspecialized_struct'

This will fail for a couple of reasons, the more important of which is how breathe does its templates. My understanding is that two separate nodes are emitted, one for the template and one for the struct unspecialized_struct, meaning find_node will only give you one.

The "right" thing to do here is probably figure out why memberdef is used for all of these tests instead of compounddef (two separate doxygen types) -- none of the tests are actually member definitions, they're all compound definitions:

<?xml version='1.0' encoding='UTF-8' standalone='no'?>
<doxygen xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="compound.xsd" version="1.8.13">
  <compounddef id="structunspecialized__struct" kind="struct" language="C++" prot="public">
    <compoundname>unspecialized_struct</compoundname>
    <includes refid="test_8hpp" local="no">test.hpp</includes>
    <templateparamlist>
      <param>
        <type>class T</type>
      </param>
      <param>
        <type>class U</type>
      </param>
    </templateparamlist>
    <briefdescription>
<para>An unspecialized template struct. </para>    </briefdescription>
    <detaileddescription>
    </detaileddescription>
    <location file="test.hpp" line="3" column="1" bodyfile="test.hpp" bodystart="3" bodyend="3"/>
    <listofallmembers>
    </listofallmembers>
  </compounddef>
</doxygen>

I can give you more information if you want, but really the point I'm trying to make here is that the current tests do not seem to support it, and I agree the PR here works well. In my opinion, since sphinx adopted pytest, and the number of tests in test_renderer are generally few, and none of the tests work in Sphinx 1.6 or higher, a more in depth discussion on revamping the testing framework should take place on a different thread 😉

@vermeeren
Copy link
Collaborator

@svenevs indeed, I noticed the tests were pretty broken on my own machine which has the latest Sphinx. Now I know why. Thank you for all this information, I didn't know most of this.

I agree that it should be a separate issue. Perhaps a new test setup will happen one day, who knows? :)

Thanks to all.

@vermeeren vermeeren merged commit f4f7a71 into breathe-doc:master May 16, 2018
@vermeeren vermeeren added enhancement Improvements, additions (also cosmetics) code Source code labels May 21, 2018
vermeeren added a commit that referenced this pull request Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code enhancement Improvements, additions (also cosmetics)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants