Skip to content

Conversation

@sitepark-schaeper
Copy link

This aims to resolve #91.

This is a rudementary implementation to figure out, what direction this feature should go.
It looks for a .pkl-module-path file in the workspacefolders, reads it, and interpretes ever line that is neither blank nor starts with a # (to allow comments) as a modulepath entry.

Currently this happens every time an import is resolved. Hence Invalid and non-existent entries are ignored at the moment to not flood the logs. A better way would probably be to use the CachedValuesManager with a dependency either to the FsFile or an lsp event signaling its creation. Alternatively it could load it just once at startup.

Also, is a .pkl-module-path file appropriate or should it be more general for lsp settings?

@HT154
Copy link
Contributor

HT154 commented May 23, 2025

I'm interested in seeing similar support for arbitrary schemes in the long run to provide better editor support when using external/custom readers. I'm curious about the core team's thoughts here.

@bioball
Copy link
Member

bioball commented May 23, 2025

Thanks for this PR!

Modulepath support needs to be a little more involved; mainly:

  • The setting should be part of configuration (see SettingsManager). On the client side, this should be configured differently depending on the editor (in VSCode, it should be defined in the extension's package.json)
  • When resolving relative paths, we should determine whether that file exists within the modulepath or not. If so, relative paths should be resolved according to the modulepath. For example, import "/foo/bar.pkl" should look within modulepath.
  • Resolving a modulepath resource should look at each jar/directory (as determined by the order of jars/dirs) and return the first file that exists. This follows the same rules as classpath resources in Java.

Feel free to explore this! But otherwise, we can look into adding support for this.

@sitepark-schaeper
Copy link
Author

The setting should be part of configuration (see SettingsManager)

I've implemented this. Clients now need a way to set this configuration individually per project.

[...] import "/foo/bar.pkl" should look within modulepath

I believe the cli would have to support this first, right?

As far as I can tell resources (from the read instruction) are not resolved at all yet and I believe that feature would be beyond the scope of this PR.

@bioball
Copy link
Member

bioball commented May 27, 2025

I believe the cli would have to support this first, right?

As far as I can tell resources (from the read instruction) are not resolved at all yet and I believe that feature would be beyond the scope of this PR.

Sorry, I probably wasn't clear enough! Relative paths are imported according to enclosing module URI. So, given this:

// enclosing module is: "modulepath:/foo.pkl"

import "/bar/baz.pkl"

The import is resolved to modulepath:/bar/baz.pkl, because the enclosing module is also a modulepath module.

So, in the LSP, we should determine if a module came from the modulepath. If so, any relative imports there should target the modulepath.

@sitepark-schaeper
Copy link
Author

Oh, thank you for clariying
But shouldn't a relative path still be relative to the file it's declared in? The modulepath may contain another equally named file, which that path should not resolve to.
As far as I can tell imports of imports are resolved correctly as it is.

I will be unavailable for the next two weeks, apologies for the inconvenience.

bioball and others added 2 commits June 16, 2025 11:58
This fixes a deadlock that arises when computing diagnostics.

This also generally reduces lock contention (the getCachedValues() API now requires a lock to be provided).
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.

support for project/package specific modulepaths

3 participants