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

Fix #7276: cpp: objects generate hypertarget names unexpectedly #7277

Merged
merged 1 commit into from Mar 15, 2020

Conversation

tk0miya
Copy link
Member

@tk0miya tk0miya commented Mar 7, 2020

Feature or Bugfix

  • Bugfix
  • Refactoring

Purpose

@jakobandersen Could you review this and give an advice to me.

@tk0miya tk0miya added this to the 3.0.0 milestone Mar 7, 2020
@codecov
Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #7277 into 3.x will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              3.x    #7277      +/-   ##
==========================================
+ Coverage   84.01%   84.05%   +0.04%     
==========================================
  Files         281      279       -2     
  Lines       43398    43221     -177     
  Branches     6329     6299      -30     
==========================================
- Hits        36459    36328     -131     
+ Misses       5559     5520      -39     
+ Partials     1380     1373       -7
Impacted Files Coverage Δ
sphinx/domains/cpp.py 80.6% <ø> (-0.01%) ⬇️
sphinx/__init__.py
sphinx/theming.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b6f06b...22084ad. Read the comment docs.

CHANGES Outdated Show resolved Hide resolved
@@ -6512,10 +6512,6 @@ def add_target_and_index(self, ast: ASTDeclaration, sig: str, signode: desc_sign
names = self.env.domaindata['cpp']['names']
if name not in names:
names[name] = ast.symbol.docname
signode['names'].append(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I can reproduce the warning this is supposed to fix, but I don't actually know where it comes from. Do you know where it is generated from? (perhaps inside docutils?) In short: besides avoiding the warning I'm actually not sure what the specific effect of removing this line is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the warning came from inside docutils.

In short: besides avoiding the warning I'm actually not sure what the specific effect of removing this line is.

In docutils world, node['names'] is used to create a hyperlink target that is able to refer via its name. On the other hand, Sphinx provides some roles (ex. :cpp:func:) to realize the cross-reference feature because docutils does not support a reference between multiple documents. Therefore, it is not needed to generate names for the hyperlink target.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, looks good to merge.

@jakobandersen
Copy link
Contributor

  • I also thought to change node_id generation mechanism to use make_id(). But cpp domain is too complicated to me. And it seems it already has its own ID generation engine. So it does not cause a problem like some domain directives generate wrong target ID #6903. So I'd like to keep as is now.

Indeed. IDs are generated from the declaration such that clashes can never happen (assuming no bugs). As features are added there has been a need to change IDs for existing objects. There are thus multiple IDs for the same entity (currently _max_id = 4), each with their own prefix to avoid clashes (see the _id_prefix variable). This makes old links still work, though some day the oldest ones should probably be removed.

@tk0miya tk0miya merged commit 504d261 into sphinx-doc:3.x Mar 15, 2020
@tk0miya
Copy link
Member Author

tk0miya commented Mar 15, 2020

Thank you for reviewing! merged.

@tk0miya tk0miya deleted the 7276_cpp_hyperlink_names branch March 15, 2020 05:13
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants