Skip to content

display class members in separate lists w/headings #181

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

Merged
merged 4 commits into from
Apr 28, 2023

Conversation

rdzman
Copy link
Contributor

@rdzman rdzman commented Apr 18, 2023

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
Copy link
Collaborator

joeced commented Apr 19, 2023

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
Copy link
Contributor Author

rdzman commented Apr 19, 2023

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
Copy link
Contributor Author

rdzman commented Apr 19, 2023

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
Copy link
Collaborator

joeced commented Apr 20, 2023

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
Copy link
Contributor Author

rdzman commented Apr 20, 2023

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
Copy link
Contributor Author

rdzman commented Apr 21, 2023

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?

Squashed commits:
[94f2ac2] autodoc_class_content "both" and "init" both add constructor docstring to class
@rdzman
Copy link
Contributor Author

rdzman commented Apr 21, 2023

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
Copy link
Collaborator

joeced commented Apr 28, 2023

@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 :)

@joeced joeced merged commit 7bcc353 into sphinx-contrib:master Apr 28, 2023
@rdzman
Copy link
Contributor Author

rdzman commented Apr 28, 2023

Thank you!

@rdzman rdzman deleted the separate-member-sections branch May 31, 2023 17:15
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.

2 participants