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

Add rosdoc2 option to specify primary_domain #24

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aprotyas
Copy link
Contributor

This option should enable sphinx to land on reasonable defaults for
code highlighting Pygments lexers, as well as other markup behaviors,
depending on the domain of a package (defaults to cpp).

Signed-off-by: Abrar Rahman Protyasha [email protected]

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.

So this PR would allow a user to provide a rosdoc2.yaml file to override and get Python highlighting. If they did not do this, then they would get C++ highlighting, even on Python packages. Is that a correct assessment of the change?

@aprotyas
Copy link
Contributor Author

aprotyas commented Aug 3, 2021

So this PR would allow a user to provide a rosdoc2.yaml file to override and get Python highlighting. If they did not do this, then they would get C++ highlighting, even on Python packages. Is that a correct assessment of the change?

That's right. There's an assumption here about the frequency of C++ packages over Python packages, which is not correct in hindsight.
If the primary_domain/highlight_domain options are not set, the default values are py and default (basically Python3) respectively. I don't know if that's ideal either as a default case for rosdoc2. If they are set to None, then of course there is no highlighting.

print(f"[rosdoc2] setting primary domain to '{{rosdoc2_settings.get('primary_domain')}}'",
file=sys.stderr)
# Tell sphinx what the primary language being documented is.
primary_domain = rosdoc2_settings.get('primary_domain')
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if the user already set primary_domain or highlight_language? We'd be overwriting them. I think it would be better to only set these if primary_domain is explicitly in rosdoc2_settings, or if the user didn't specify primary_domain or highlight_language.

Copy link
Contributor Author

@aprotyas aprotyas Aug 6, 2021

Choose a reason for hiding this comment

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

With the changes in ac4a5ea, the last talking point is about a reasonable default for the domain/highlighting - check #24 (comment). The following snippet should then do the trick:

ensure_global('primary_domain', {default_language})
ensure_global('highlight_language', {default_language})

if rosdoc2_settings.get('primary_domain'):
    ...

Of course, the ensure_global calls can be skipped altogether if we settle that the sphinx defaults of py/default for primary_domain/highlight_language are reasonable defaults for rosdoc2's use.


Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I wouldn't default to cpp I think. The user can always explicitly label the language of code in their doc blocks right? Or does this cover a case where that doesn't work?

If we do pick a default other than the sphinx default, I'd say we should do cpp if it is an ament_cmake package and py or do nothing if it is an ament_python package.

Copy link
Contributor Author

@aprotyas aprotyas Aug 6, 2021

Choose a reason for hiding this comment

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

Or does this cover a case where that doesn't work?

Off the top of my head, I don't think this would cover any case where the user couldn't explicitly label the code block.

I'd say we should do cpp if it is an ament_cmake package and py or do nothing if it is an ament_python package.

Alright, I will keep this in mind as I'm working on resolving #19.

rosdoc2/verbs/build/builders/sphinx_builder.py Outdated Show resolved Hide resolved
@aprotyas
Copy link
Contributor Author

aprotyas commented Aug 6, 2021

In the case that the user specifies neither a primary_domain/highlight_language in their conf.py configuration nor a primary_domain in rosdoc2_settings (i.e. it is not the case that the user's setting will be overwritten), is there a reasonable default for the domain/language that rosdoc2 could/should configure? The sphinx default value for these options is py/default respectively, which is essentially a pseudo-Python3 syntax. I'm leaning towards a cpp default, but I can't back that up with a relative frequency of ament_cmake packages versus that of ament_python packages comparison

Thoughts?

This option should enable `sphinx` to land on reasonable defaults for
code highlighting Pygments lexers, as well as other markup behaviors,
depending on the domain of a package (defaults to `cpp`).

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas aprotyas force-pushed the aprotyas/add-primary-domain-option branch from 598d38b to f3ebfb6 Compare August 6, 2021 03:59
Previously, this setting defaulted to `cpp`, which is not ideal
because only be false if the user explicitly did
`rosdoc2_settings = {'primary_domain': None}` or similar.

This commit changes that default from `cpp` to `None`, which is more
reasonable and closer to expected behavior from the user's POV.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Copy link
Contributor Author

aprotyas commented Aug 9, 2021

With ac4a5ea, the current behavior is to not change the primary domain setting in any way unless explicitly specified. However, having extracted build type information in #28, it would make more sense to set the primary domain to cpp/py depending on whether the build type is ament_cmake/ament_python (suggestion in #24 (comment)). This information can be format mapped to rosdoc2_wrapping_conf_py_template through a template variable.

Thoughts?

@wjwwood
Copy link
Collaborator

wjwwood commented Aug 10, 2021

However, having extracted build type information in #28, it would make more sense to set the primary domain to cpp/py depending on whether the build type is ament_cmake/ament_python (suggestion in #24 (comment)).

Yes I think so, I was imaging cpp for ament_cmake and not setting anything (using the default) for ament_python. But if the standard practice for Python projects is to manually set it to py then that would be ok for ament_python as well. Basically I think ament_python should do whatever is "normal" for a plain python package.

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.

3 participants