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 broken references #1317

Closed
wants to merge 12 commits into from
Closed

fix broken references #1317

wants to merge 12 commits into from

Conversation

tpoint75
Copy link

@tpoint75 tpoint75 commented May 9, 2023

No description provided.

@12rambau
Copy link
Collaborator

12rambau commented May 9, 2023

What is the difference between the anchorname and refid parameter of a node ?

@tpoint75
Copy link
Author

Your fix for the scrollspy breaks the internal links. It generates he format this.is.the.webpage.html#and_this_is_theAnchor ..
I inspected the broken nodes and fixed it. But I don't know, if I have done it right with the double try catch.
I found the 'refuri' and 'refid' tags to be used. Each anchor element also has the 'refuri'.

@tpoint75
Copy link
Author

The links from the search results are also broken and does not respect the underscores!

@tpoint75
Copy link
Author

search bar fixed in 6ba1624

@drammock drammock added kind: bug Something isn't working impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship labels Jun 21, 2023
@drammock
Copy link
Collaborator

@tpoint75 can you please install our pre-commit hook, run it on this PR, and push the results? There are persistent failures of our style checks, which (if using pre-commit) are detected and fixed before pushing to GitHub.

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

@tpoint75 sorry for the verry long wait, I think I now have time to review your PR.

  • Could you rebase it on master (we had some issues with Sphinx 7 that seems to still be included to your branch)
  • the searchtoold.js file that you copied is coming directly from Sphinx right ? what did you changed to make it work here ?
  • how do you override the original file ?

Comment on lines +118 to +119
first = parts[0]
target_id = parts[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
first = parts[0]
target_id = parts[1]
first, target_id = parts

Comment on lines +115 to +116
first = ""
target_id = refid
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
first = ""
target_id = refid
first, target_id = "", refid

sanitized_id = target_id.replace(".", "_")
# Update the node `href`
node["refuri"] = node["anchorname"] = "#" + sanitized_id
node["refuri"] = first + "#" + sanitized_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't you update anchorname as well ?

Comment on lines +101 to +108
conditions = (
("refuri", node.get("internal", False)),
("refid", True),
("refuri", True),
)
for key, _internal in conditions:
refid = node.get(key, "")
if _internal and "." in refid:
Copy link
Collaborator

Choose a reason for hiding this comment

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

whatever the value of node.get("internal", False) the refuri key will be tested as True in l108 is it normal ? If yes why not simply use a list: ["refid", "refuri"] ?

@12rambau 12rambau removed kind: bug Something isn't working impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship labels Sep 15, 2023
@drammock
Copy link
Collaborator

after #1438 (which rolled back #1208) I think this PR is moot / no longer needed.

@drammock drammock closed this Sep 15, 2023
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.

None yet

3 participants