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

Revamp default Sphinx project #33

Merged

Conversation

hidmic
Copy link
Collaborator

@hidmic hidmic commented Sep 30, 2021

This patch:

  • Modifies default index
  • Uses Exhale generated index
  • Changes Exhale configuration

Requires svenevs/exhale#114.

@hidmic
Copy link
Collaborator Author

hidmic commented Sep 30, 2021

FYI @clalancette. BTW, this project is not passing its own tests in main.

Comment on lines 95 to 96
"customSpecificationsMapping": utils.makeCustomSpecificationsMapping(
lambda kind: [":content-only:", ":no-link:"] if kind == "page" else []),
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, this also changes the behavior when there is a different (not :doxygenpage:) directive. As in, other directives such as :doxygenfunction: wouldn't render correctly with this mapping, and I think you have to provide mappings for each such directive. I will confirm whether that's true in a future comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 in general for the behavior this should give otherwise though.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-10-28/22947/1

@hidmic
Copy link
Collaborator Author

hidmic commented Nov 9, 2021

This is ready to go @clalancette.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-11-18/23209/1

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Now that we have the support in upstream exhale, we can move forward with this. I have one suggestion inline

}

with open(os.path.join(directory, 'conf.py'), 'w+') as f:
f.write(default_conf_py_template.format_map(template_variables))

root_title = f'Welcome to {package.name} documentation'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would read better as:

Welcome to the documentation for {package.name}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. See d942673.

# TODO(aprotyas): Uncomment the mapping below once the above issue is resolved.
# "customSpecificationsMapping": utils.makeCustomSpecificationsMapping(
# lambda kind: [":project:", ":path:", ":content-only:"] if kind == "page" else []),
"customSpecificationsMapping": utils.makeCustomSpecificationsMapping(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we ever resolve @aprotyas issue with this? Quoting:

IIRC, this also changes the behavior when there is a different (not :doxygenpage:) directive. As in, other directives such as :doxygenfunction: wouldn't render correctly with this mapping, and I think you have to provide mappings for each such directive. I will confirm whether that's true in a future comment.

Copy link
Collaborator Author

@hidmic hidmic Dec 6, 2021

Choose a reason for hiding this comment

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

It's funny, exhale documentation is misleading here. If utils.makeCustomSpecificationsMapping yields an empty list for the specs of given directive, it'll use Breathe defaults, which are not the same as Exhale defaults (https://github.com/svenevs/exhale/blob/bbb5d71f126d1281a8c4f1729c218c236f44457b/exhale/utils.py#L185). I can change this to match Exhale defaults though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me whether we should be going for the Breathe or Exhale defaults in that case. @hidmic @aprotyas I would take whatever your recommendation here is.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Exhale defaults are not bad, but those defaults are only for the class and struct kinds.
It would be nice to pay the one-time cost of a rosdoc2_specsMapping dictionary that lets us control the directive of all kinds that are of interest. It also makes for more extensibility:

rosdoc2_specsMapping = {                                                                               
    'page': [':content-only:'],                  
    **dict.fromkeys(['class', 'struct'], [':members:', ':protected-members:', ':undoc-members:']),  
    # other kind-specs mapping                                                                         
}       

"customSpecificationsMapping": utils.makeCustomSpecificationsMapping(
    lambda kind: rosdoc2_specsMapping.get(kind, [])),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That looks reasonable to me. See befbade.

@hidmic hidmic requested a review from clalancette December 6, 2021 22:29
- Modify default index
- Use Exhale generated index
- Change Exhale configuration

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
- Expose exhale specs mapping as a rosdoc2 setting
- Use exhale defaults as rosdoc2 defaults

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the revamp-default-sphinx-project branch from befbade to 3bd61d0 Compare December 7, 2021 22:12
@hidmic
Copy link
Collaborator Author

hidmic commented Dec 7, 2021

Rebased and ready for another round of reviews.

@clalancette clalancette merged commit a3d6885 into ros-infrastructure:main Dec 9, 2021
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.

4 participants