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

Cpp domain improvements #64

Merged
merged 11 commits into from
Apr 24, 2022
Merged

Cpp domain improvements #64

merged 11 commits into from
Apr 24, 2022

Conversation

jbms
Copy link
Owner

@jbms jbms commented Apr 21, 2022

This adds a number of improvements for the C++ domain:

  • Improved styling
  • clang-format-based signature formatting
  • Support for resolving cpp domain references to external links, with specific support for cppreference.com
  • Support for #include directives in C++ signatures to indicate the required header
  • Support for cross-linking parameters in C and C++ domains
  • Support for "synopses" of C/C++ domain objects shown in tooltips and search results
  • Option for including object description fields in TOC

@jbms jbms requested a review from 2bndy5 April 21, 2022 20:13
@jbms jbms force-pushed the cpp-domain-improvements branch 3 times, most recently from 256266d to 5818d13 Compare April 21, 2022 21:01
Copy link
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

First glance

I'll dive into the src changes later. This is just what I noticed in the docs changes.

docs/external_cpp_references.rst Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
docs/external_cpp_references.rst Show resolved Hide resolved
docs/format_signatures.rst Show resolved Hide resolved
docs/format_signatures.rst Outdated Show resolved Hide resolved
docs/format_signatures.rst Outdated Show resolved Hide resolved
docs/index.rst Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@2bndy5 2bndy5 linked an issue Apr 22, 2022 that may be closed by this pull request
docs/cpp.rst Outdated Show resolved Hide resolved
@jbms jbms force-pushed the cpp-domain-improvements branch 2 times, most recently from 353a1c7 to 0db39ef Compare April 22, 2022 14:52
docs/cpp.rst Show resolved Hide resolved
@jbms jbms force-pushed the cpp-domain-improvements branch 7 times, most recently from 724a9ed to fb76c7d Compare April 22, 2022 16:14
docs/format_signatures.rst Outdated Show resolved Hide resolved
@jbms jbms force-pushed the cpp-domain-improvements branch 3 times, most recently from 6c9a245 to 2b71ddc Compare April 23, 2022 22:51
@jbms
Copy link
Owner Author

jbms commented Apr 23, 2022

Thanks for your review @2bndy5 --- I believe I have now addressed all of your comments.

I also added some new functionality --- synopses of C/C++ domain objects displayed as tooltips and in search results.

@jbms
Copy link
Owner Author

jbms commented Apr 24, 2022

Wow! The description fields feature is something I've dreamed of for years. And, you just dished it out like a side entree rofl . I'm reviewing those changes now - everything else looks good.

To be clear, include_object_description_fields_in_toc just controls whether "Parameters", "Returns", etc. end up in the TOC.

Whether the individual parameters themselves end up in the TOC is independent of that currently, and requires domain-specific support --- this PR adds support for that to the C++ domain (in the cross-link parameters commit). And in the tensorstore docs I also do that for Python and JSON, but those changes aren't part of this theme yet.

However, it looks a bit weird to have the individual parameters in the TOC without a "Parameters" heading.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 24, 2022

However, it looks a bit weird to have the individual parameters in the TOC without a "Parameters" heading.

I am seeing a section for Parameters and such
in the python domain
image
and in the C++ domain
image

docs/api.rst Show resolved Hide resolved
docs/cpp.rst Show resolved Hide resolved
docs/cpp.rst Show resolved Hide resolved
@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 24, 2022

I've noticed that the tooltips for confval references no longer show up.

I wasn't aware that they had tooltips --- can you explain a bit more what you mean?

I must have been mis-remembering this. I thought that hovering over a confval ToC entry or inline xref would yield a tooltip like "option_name (confval)". But, I can't seem to find an example of this anywhere (not even in sphinx-doc.org). Please disregard.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 24, 2022

The param direction for C++ domain looks a bit sequestered.
image
Especially for wide viewports.

I couldn't devise a CSS way to replace the floatr: right property. My idea was either append to the end of a param name or use a nested dl in which the dt is the param direction and the dd is the param name(s).

@jbms
Copy link
Owner Author

jbms commented Apr 24, 2022

The param direction for C++ domain looks a bit sequestered. image Especially for wide viewports.

I couldn't devise a CSS way to replace the floatr: right property. My idea was either append to the end of a param name or use a nested dl in which the dt is the param direction and the dd is the param name(s).

I am also not too satisfied with the appearance of this, but I couldn't figure out a better alternative. Perhaps could be revised in the future.

Copy link
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

As always these changes go above and beyond my expectations! Thank you for this!

@jbms jbms merged commit 230a039 into main Apr 24, 2022
@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 24, 2022

I'm going to tag the master branch head, but I wanted to ask if you think this is a minor version bump or a patch bump. I've been using minor version bumps for merges from upstream, so I'm leaning toward a patch version bump.

@jbms jbms deleted the cpp-domain-improvements branch April 24, 2022 05:39
@jbms
Copy link
Owner Author

jbms commented Apr 24, 2022

In general patch -> bug fixes only, while minor -> backwards compatible feature addition. I think if we synchronized our version numbers with mkdocs-material then a patch bump could make sense, but so far we have not done that, and as we add more independent sphinx-related functionality to this theme (such as in this PR), the upstream changes from mkdocs-material may not necessarily be the most significant changes. So I think minor version bump would make sense as well here, and maybe we won't bother to use a patch bump.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 24, 2022

Well I pushed a v0.5.0 tag, and setuptools_scm interpreted it as 0.5.0.post1.dev0 which got treated as a pre-release on pypi. So, fresh installs will still be getting v0.4.1. Pushing a tag v0.5 (after v0.5.0 tag was pushed) wasn't uploaded to pypi (only test-pypi).

I'm guessing the post1.dev0 is indicative of pre-release status on pypi, but I don't know what I could've done to prevent that from being appended.

jbms added a commit that referenced this pull request Apr 24, 2022
Previously, binary files like .gif were being incorrectly converted,
leading to a dirty working tree and `post` version numbers selected by
setuptools_scm.

#64 (comment)
@jbms jbms mentioned this pull request Apr 24, 2022
@jbms
Copy link
Owner Author

jbms commented Apr 24, 2022

I have a fix for the version number issue. I removed the v0.5.0 tag and the post release from pypi --- we can push a new v0.5.0 tag once the PR is merged.

jbms added a commit that referenced this pull request Apr 24, 2022
Previously, binary files like .gif were being incorrectly converted,
leading to a dirty working tree and `post` version numbers selected by
setuptools_scm.

#64 (comment)
jbms added a commit that referenced this pull request Apr 25, 2022
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.

multilined cpp function signatures split between param name and datatype
2 participants