-
Notifications
You must be signed in to change notification settings - Fork 13
Add configurable link to pkl doc in hover #104
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
base: main
Are you sure you want to change the base?
Conversation
gordonbondon
commented
Sep 23, 2025
| val packageDocumentationUrls: Map<String, String> = | ||
| mapOf( | ||
| DEFAULT_PKL_AUTHORITY to | ||
| "https://pkl-lang.org/package-docs/{packagePath}/{version}/{modulePath}/{path}" |
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.
We shouldn't need users to provide any placeholders here. The relative path within a docsite can be derived by the module/class/method/property name.
It's always going to be:
pkgName.replace(".", "/") + "/" + version + "/" + moduleName.replace(".", "/") + ("index" | "classname") + ".html" + fragment?
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.
The LSP doesn't really know where a package's documentation might exist. It might make sense to just accept a list of docsite URLs, where pkl-lsp tries each one to see if documentation exists. But, if we do this, we'd need some way to cache this so that we aren't hammering the documentation site needlessly.
Not all packages from pkg.pkl-lang.org exist in pkl-lang.org/package-docs. For example, packages published by users can be fetched from there, but we only publish package docs for the packages that we maintain ourselves.
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.
How about we have a config with package/module prefixes mapped to doc site url prefix?
Something like:
pkg.pkl-lang.org: pkl-lang.org/package-docs
pkg.pkl-lang.org/pkl-go: example.com/docs/
pkg.pkl-lang.org/pkl-go/pkl.golang: another.example.com/my-doc-root/
Default will be:
pkg.pkl-lang.org: pkl-lang.org/package-docs
Trying multiple urls and caching them for each hover seems like an overhead.
Looking at how other extensions handle this:
- vscode-go allows only one base hsotname for docs https://github.com/golang/vscode-go/blob/ddc8268c35ff1d0a1c8ae320e34a20d2063bf687/extension/package.json#L3278 , private packages are not supported at all (the ones matching GOPRIVATE)
- actually did not find any other language extensions returning a link to docsite : (
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.
For future improvement, how about having a url shema for docs similar to source code schema in package description? https://pkl-lang.org/package-docs/pkl/current/Project/Package.html#sourceCodeUrlScheme
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.
godoc is a central docsite (all public go modules are available on godoc).
Currently, we don't have the concept of a central pkldoc site, but this is something that we've talked about hosting in the past.
If we have that, then we only need one setting for "this is where the docsite lives".
How about we have a config with package/module prefixes mapped to doc site url prefix?
For the pkg.pkl-lang.org stuff, we kind of need a "negative match" on packages that start with pkg.pkl-lang.org/github.com, rather than a prefix match.
For future improvement, how about having a url shema for docs similar to source code schema in package description? https://pkl-lang.org/package-docs/pkl/current/Project/Package.html#sourceCodeUrlScheme
The package metadata actually does have a field for documentation: https://pkl-lang.org/package-docs/pkl/current/Project/Package.html#documentation. This field will appear in the package metadata if set.
Perhaps, for now, we can just use that field in lieu of first checking a docsite.
| moduleFileUri.startsWith("jar:") -> { | ||
| // Extract the path after the zip file reference | ||
| val afterJar = moduleFileUri.substringAfter("!/") | ||
| afterJar.removeSuffix(".pkl") | ||
| } |
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.
The documentation path is always determined by the module name, not the path within the package
| moduleFileUri.startsWith("jar:") -> { | |
| // Extract the path after the zip file reference | |
| val afterJar = moduleFileUri.substringAfter("!/") | |
| afterJar.removeSuffix(".pkl") | |
| } |
In the case of the module name, we strip off the package's display name from the path.
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.
With this change it gets me:
"https://pkl-lang.org/package-docs/pkg.pkl-lang.org/pkl-k8s/k8s/1.2.1/k8s.k8sSchema/index.html"
instead of expected:
"https://pkl-lang.org/package-docs/pkg.pkl-lang.org/pkl-k8s/k8s/1.2.1/k8sSchema/index.html"
So k8s.k8sSchema instead of k8sSchema for module name.
| val packageUri = packageDep.packageUri | ||
| val authority = packageUri.authority | ||
|
|
||
| authority + "/" + packageUri.path.substringBeforeLast('@').removePrefix("/") |
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.
Make org.pkl.lsp.packages.dto.PackageUri#basePath public, and use that instead of packageUri.path.substringBeforeLast('@')
| when (node) { | ||
| is PklModule -> module.moduleName | ||
| is PklProperty -> node.name | ||
| is PklMethod -> node.methodHeader.identifier?.text?.let { "$it()" } ?: "method" |
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.
We already have PklMethodHeader here, PklMethod is the whole method include the method body
| is PklMethod -> node.methodHeader.identifier?.text?.let { "$it()" } ?: "method" |
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.
With this removed link text for me instead of hasUniquePortNames() defaulted to a module name k8s.K8sObject so PklMethodHeader is not matching, in test case:
local result = new K8sObject {}.hasUnique<caret>PortNames()
| try { | ||
| java.net.URI(docUrl).host | ||
| } catch (e: Exception) { | ||
| "documentation" |
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 would be a misconfiguration error if this happens; seems like we should catch this at configuration time.
Co-authored-by: Daniel Chao <[email protected]>