sphinx-contrib / matlabdomain

A Sphinx extension for documenting Matlab code
http://sphinxcontrib-matlabdomain.readthedocs.io/
Other
69 stars 45 forks source link

display class members in separate lists w/headings #181

Closed rdzman closed 1 year ago

rdzman commented 1 year ago

Here is one approach to addressing #179.

As I looked at the code in MatClassDocumenter.document_members(), it occurred to me that one could still take advantage of the superclass method by "borrowing" the exclude_members option and calling the superclass document_members() multiple times with different subsets excluded.

I know this is a bit of a hack, but it does generate roughly the result I'm looking for.

What do you think of this approach?

Screenshot 2023-04-18 at 3 10 23 PM

joeced commented 1 year ago

This looks really nice. I really like how you figured out how the rendering works, that's always been a bit of dark magic for me. We just have to get the tests to pass then. Do you have time to go over them.

rdzman commented 1 year ago

I'm happy to work on getting the tests passing assuming you are ok with the approach and implementation as is. Any suggested changes before I do?

And for the tests, it seems most are based on loading content via pickle and then comparing a portion astext() against some expected output. I'm not (yet) familiar with the structure of content and I'm not sure what's the easiest way to grab a copy of the updated expected value to paste into the test script. Any tips?

rdzman commented 1 year ago

I pushed an update that cleans up the code a bit.

I'm starting to work on the tests and realized, after seeing an empty constructor section show up on the first test, that I was missing some things. I want to make sure we can avoid outputting a section header if there are no corresponding items that will be displayed in that section.

So, for one thing, unless we have :undoc-members:, it looks like I'll have to dive into the objects themselves to see if the member has a docstring and should therefore be included.

But if autoclass_content is set to init or both then any constructor docstring would already have been displayed for the class, right? Should we have it skip the constructor in that case too?

It also appears the class signature is always pulled from the constructor. It might be nice to have an option to simply use the name as the class signature for the case where the constructor is displayed separately. What do you think? But I suppose that's a separate issue.

joeced commented 1 year ago

Very nice that you made progress. Regarding testing with the text output, I usually debug it and check the output that is generated and verify manually that it is correct.

I pushed an update that cleans up the code a bit.

I'm starting to work on the tests and realized, after seeing an empty constructor section show up on the first test, that I was missing some things. I want to make sure we can avoid outputting a section header if there are no corresponding items that will be displayed in that section.

I like that you don't want to output empty sections. Please add that :)

So, for one thing, unless we have :undoc-members:, it looks like I'll have to dive into the objects themselves to see if the member has a docstring and should therefore be included.

But if autoclass_content is set to init or both then any constructor docstring would already have been displayed for the class, right? Should we have it skip the constructor in that case too?

Yes, with your approach (which is so close to MathWorks👍), it does not really makes sense does it? We show the class docstring and we show the constructor docstring and signature in the Constructor Summary.

It also appears the class signature is always pulled from the constructor. It might be nice to have an option to simply use the name as the class signature for the case where the constructor is displayed separately. What do you think? But I suppose that's a separate issue.

I'm not sure I understand this. Can you give an example

rdzman commented 1 year ago

It also appears the class signature is always pulled from the constructor. It might be nice to have an option to simply use the name as the class signature for the case where the constructor is displayed separately. What do you think? But I suppose that's a separate issue.

I'm not sure I understand this. Can you give an example

For a class myClass whose constructor takes two arguments arg1 and arg2, right now it displays something like ...

class myClass(arg1, arg2)

    <class docstring content>

    Constructor Summary
        myClass(arg1, arg2)

        <constructor docstring content>

I'm suggesting we might want an option that simply removes the arguments from the class signature, so it would yield something like ...

class myClass

    <class docstring content>

    Constructor Summary
        myClass(arg1, arg2)

        <constructor docstring content>
rdzman commented 1 year ago

So, for one thing, unless we have :undoc-members:, it looks like I'll have to dive into the objects themselves to see if the member has a docstring and should therefore be included. But if autoclass_content is set to init or both then any constructor docstring would already have been displayed for the class, right? Should we have it skip the constructor in that case too?

Yes, with your approach (which is so close to MathWorks👍), it does not really makes sense does it? We show the class docstring and we show the constructor docstring and signature in the Constructor Summary.

I recommend we ignore the value of autoclass_content completely, and always use the class docstring for the class and the constructor docstring for the constructor (which will appear under Constructor Summary if present). Is that what you were suggesting?

rdzman commented 1 year ago

On second thought, I don't find autoclass_content useful and will always use the default value, but I don't suppose there's any reason to change the current behavior.

I think I'm happy with this PR if you are.

I'll create a separate issue about the format of the class signature.

joeced commented 1 year ago

@rdzman This is a really, really nice contribution. Thanks a lot. I'll merge this to master before #171, to avoid you having to deal with the merge conflicts :)

rdzman commented 1 year ago

Thank you!