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: Make docs links passthrough #1085

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

jnumainville
Copy link
Collaborator

Fixes https://deephaven.atlassian.net/browse/DOC-355 by "making all links external", meaning they are passthrough instead of being processed by myst parser. In particular the absolute paths were considered invalid because they were file locations that didn't exist at build time.

@@ -39,6 +39,8 @@
# options for sphinx_autodoc_typehints
always_use_bars_union = True

myst_all_links_external = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think instead of this we should add a URL scheme for absolute paths. See docs. I think we could just add '/' as a scheme to match any URL starting with a slash and effectively skip checking all absolute URLs without skipping checking internal URLs

Copy link
Collaborator

@mattrunyon mattrunyon Jan 16, 2025

Choose a reason for hiding this comment

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

Nevermind that might require a colon like /:path to work. The other option would be we add a custom scheme like dh: to prefix external links and start failing on myst failures. Probably should so we don't have bad links internally

Copy link
Collaborator Author

@jnumainville jnumainville Jan 16, 2025

Choose a reason for hiding this comment

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

I did consider a custom scheme but seemed a bit annoying to have to keep track of if it can be avoided. But if we want to do that as a way to validate I'd be fine with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ya it's a hard pick because salmon doesn't rewrite any absolute links. So absolute links can't be to actual pages within the project because they won't work on salmon. /components/button.md would resolve to deephaven.io/components/button which won't exist.

In my ideal scenario absolute links always fail and require the schema. But it didn't look like myst worked that way and I'm not sure it's worth the effort it might take to enforce that.

I'd still prefer internal links be validated. We have broken links in the current docs I think, but they don't fail doc builds. They should fail doc builds

Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

I'll approve for now, but would like to revisit checking internal links since we currently have some that are broken according to myst

@jnumainville
Copy link
Collaborator Author

Going to go ahead and merge, but we should leave https://deephaven.atlassian.net/browse/DOC-355 open as this is only a partial solution

@jnumainville jnumainville merged commit 2ef0ddb into deephaven:main Jan 31, 2025
26 checks passed
mattrunyon pushed a commit to mattrunyon/deephaven-plugins that referenced this pull request Feb 3, 2025
Fixes https://deephaven.atlassian.net/browse/DOC-355 by "making all
links external", meaning they are passthrough instead of being processed
by myst parser. In particular the absolute paths were considered invalid
because they were file locations that didn't exist at build time.
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.

2 participants