-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: main
Are you sure you want to change the base?
Add rosdoc2
option to specify primary_domain
#24
Conversation
There was a problem hiding this 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?
That's right. There's an assumption here about the frequency of C++ packages over Python packages, which is not correct in hindsight. |
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') |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 anament_cmake
package andpy
or do nothing if it is anament_python
package.
Alright, I will keep this in mind as I'm working on resolving #19.
In the case that the user specifies neither a 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]>
598d38b
to
f3ebfb6
Compare
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]>
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 Thoughts? |
Yes I think so, I was imaging |
This option should enable
sphinx
to land on reasonable defaults forcode 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]