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

docstring: fix handling of empty documentation comments #167

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

jnikula
Copy link
Owner

@jnikula jnikula commented Jul 5, 2023

Sometimes it's handy to add an empty documentation comment to generate documentation for a symbol without having to add some placeholder text. Or to include documentation for members/enumerations without documenting the struct/enum.

Currently, empty documentation comments lead to:

File "docstring.py", line 143, in _remove_comment_markers
while (not lines[0] or lines[0].isspace()):
~~~~~^^^
IndexError: list index out of range

Fix it and add some tests and documentation for it.

@jnikula jnikula requested a review from BrunoMSantos July 5, 2023 09:24
doc/syntax.rst Outdated

It's also a way to let Sphinx know about types to highlight them in
e.g. parameter documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether this is necessary at all, but I don't object to it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can leave that part out. I was contemplating leaving out the entire documentation on empty comments, but thought there's no harm.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the whole section too by the way. I see no point in documenting this at all, I think anyone would expect this behaviour intuitively.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just one sentence in the previous section?

"Empty documentation comments /***/ create the C or C++ Domain directives without documentation."

Or nothing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you... I don't feel it's needed but I'd prefer a single sentence to a whole section.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped it completely. Can be added later if needed.

struct empty_comment_3 {
/** Another empty documentation comment. */
int description;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about top level comments? Any chance it might be an issue for autosection? (I plead ignorance by the way.)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not try yet. But seems like it could be a feature! You could use it to end a section (the future version with :members: support).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahahah, maybe...

I'm expecting it to work fine right now, but I'd welcome a unit test for selecting sections before and after one of these at least. In case we inadvertently break it later on.

Copy link
Owner Author

@jnikula jnikula Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some top level comments in there, with a c:autosection test. Good enough?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, feels complete now. Thanks.

@BrunoMSantos
Copy link
Collaborator

Interesting edge case. I think it's good over the covered cases and half expecting my worry about top level comments is unfounded. In which case, looks good to me.

Out of curiosity, is this just a pile of things you had laying around or is it being currently driven by actual use cases / requests?

@jnikula
Copy link
Owner Author

jnikula commented Jul 5, 2023

Interesting edge case. I think it's good over the covered cases and half expecting my worry about top level comments is unfounded. In which case, looks good to me.

Out of curiosity, is this just a pile of things you had laying around or is it being currently driven by actual use cases / requests?

I was hacking on Mesa documentation a bit, see https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23145#note_1985368

There were cases where Doxygen+Breathe happily generated documentation if struct members were documented but the struct itself was not. I'm not sure I like to promote that model in Hawkmoth, but I thought adding an empty documentation comment to the enclosing struct would be a nice workaround instead of having to add placeholder text (especially because I was unable to come up with actual documentation in the conversion). And it promptly crashed. 🤦

The warning improvements came from there as well. There were directives with typoed/wrong names or filenames, and there was no warnings, just silently no documentation.

Sometimes it's handy to add an empty documentation comment to generate
documentation for a symbol without having to add some placeholder
text. Or to include documentation for members/enumerations without
documenting the struct/enum.

Currently, empty documentation comments lead to:

  File "docstring.py", line 143, in _remove_comment_markers
    while not lines[0] or lines[0].isspace():
              ~~~~~^^^
IndexError: list index out of range

Fix it and add some tests for it. Mix in some empty and non-empty top
level comments to ensure they still work.
@jnikula jnikula force-pushed the empty-documentation-comments branch from 668eca5 to 02c5119 Compare July 5, 2023 19:58
@jnikula jnikula merged commit ba6165f into master Jul 5, 2023
4 checks passed
@jnikula jnikula deleted the empty-documentation-comments branch July 5, 2023 20:30
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

2 participants