-
Notifications
You must be signed in to change notification settings - Fork 51
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 a fullyQualifiedTitles option #188
base: master
Are you sure you want to change the base?
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.
Sorry for the delay, thanks so much for implementing this new feature @LizardM4!
I'm happy with the feature as-is, but there could be something smarter we could do here. I will want to add some tests, hoping to do that soon. Please rebase this PR so I can run CI (other contributors helped me get things back up and running with #190).
I don't think I can add commits to your branch which is OK, I will include testing patch file and can squash merge everything at the end.
@@ -1505,6 +1522,7 @@ def apply_sphinx_configurations(app): | |||
("treeViewBootstrapLevels", int), | |||
# Page Level Customization | |||
("includeTemplateParamOrderList", bool), | |||
("fullyQualifiedTitles", bool), |
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.
there is another option that comes to mind, three modes: "default", "never", "always".
- default: use shorter name if no overloaded name exists. that data is parsed elsewhere, I would be able to help with this.
- never: always use short title
- always: always use fully qualified title
I'm OK with just keeping it "on/off" for simplicity, but figured I'd see what your thoughts are on it before we continue forward.
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.
This seems a nice extension, provided that the TOC can stay sorted consistently, e.g.
- Struct abc (in ns1::)
- Struct ns1::foobar
- Struct cde (in ns2::)
- Struct ns2::foobar
Otherwise I fear it might be hard to browse. However the order of appearance should be set by exhale, correct?
Concerning the overloaded names, is such data available (specifically from initializeNodeFilenameAndLink
)? If you can point me to that I can try out some change. Perhaps we should refer to this as "clashing names", since "overloads" in C++ refer to something else (and also wouldn't be able to identify structures with the same name in different namespaces).
parent=node.parent.name.split("::")[-1], | ||
child=title | ||
) | ||
# name without namespaces for clarity even if the titles are not fully qualified |
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.
todo(@svenevs) this comment needs work
also bookmark for me: add tests
@@ -500,6 +500,8 @@ something like this, where special treatment is given to File pages specifically | |||
|
|||
.. autodata:: exhale.configs.includeTemplateParamOrderList | |||
|
|||
.. autodata:: exhale.configs.fullyQualifiedTitles |
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.
not for this file, but in docs/changelog.rst
could you please add an entry?
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 71719be..29c5c1a 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -5,6 +5,12 @@ Changelog
:local:
:backlinks: none
+v0.3.7
+----------------------------------------------------------------------------------------
+- Add :data:`~exhale.configs.fullyQualifiedTitles` option so that pages like
+ ``device1::error`` and ``device2::error`` render as unique names (:issue:`187`,
+ :pr:`188`).
+
v0.3.6
----------------------------------------------------------------------------------------
or something similar, just link to new config entry and the issue / pr 🙂
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.
Sure, done.
By default, the behavior is unchanged, however when set to True, all page titles for structures, unions, classes etc include the namespace as a prefix.
5c9dec2
to
dd75875
Compare
Hi @svenevs, thanks for getting back on this! I have rebased the branch, and you should be able to push to the fork as well. I'll reply to the individual conversation. |
Closes #187.
By default, the behavior is unchanged, however when set to True, all page titles for structures, unions, classes etc include the namespace as a prefix.