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 links to dependencies #524

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

bioball
Copy link
Contributor

@bioball bioball commented Jun 13, 2024

This fixes an issue where package dependencies are not rendered correctly.

This also changes the names shown for these dependencies.

Before: fruits
After: localhost:0/fruits

This fixes an issue where package dependencies are not rendered correctly.
pageScope.relativeSiteUrl
.resolve(
"${localSitePackage.docPackageInfo.name.pathEncoded}/current/index.html"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will only show a local link if the package is known to Pkldoc, and it will link to "current" rather than the depended upon version. If the package is not known, and we don't know what its documentation URL is, this will just display plain text.

Note: I considered making this link directly to the version, e.g. "${localSitePackage.docPackageInfo.name.pathEncoded}/${localSitePackage.docPackageInfo.version.pathEncoded}/index.htm

However, we don't know if that version exists in the doc site. The page generator doesn't know about all the versions of each package, and we don't build this index until after the static HTML has been built.

Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

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

No blockers; one possible smell

Comment on lines +77 to +78
name =
"${dependency.packageUri.uri.authority}${dependency.packageUri.uri.path.substringBeforeLast('@')}",
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a bit of a smell to it. Is the definition of name not adrift this way?

@bioball
Copy link
Contributor Author

bioball commented Jun 13, 2024

This actually introduces a new problem--links to modules of a package dependency is now broken. I'm going to hold off on this for now.

@bioball bioball marked this pull request as draft June 13, 2024 16:50
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