-
Notifications
You must be signed in to change notification settings - Fork 203
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 Environment vars and empty daml.yamls in multi-ide #20306
Conversation
@@ -79,7 +79,7 @@ export async function activate(context: vscode.ExtensionContext) { | |||
// Try to find a client for the virtual resource- if we can't, log to DevTools | |||
let foundAClient = false; | |||
for (let projectPath in damlLanguageClients) { | |||
if (vrPath.startsWith(projectPath)) { | |||
if (isPrefixOfVrPath(projectPath)) { |
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.
Small fix missing from this pr
@@ -221,7 +221,7 @@ async function startLanguageServers(context: ExtensionContext) { | |||
(await fileExists(rootPath + "/.envrc")) && | |||
vscode.extensions.getExtension("mkhl.direnv") == null; | |||
|
|||
if (envrcExistsWithoutExt && gradleSupport) { | |||
if (envrcExistsWithoutExt) { |
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.
People use .envrc
without gradle, this message should not be restricted to that
@@ -285,7 +285,7 @@ async function startLanguageServers(context: ExtensionContext) { | |||
} else { | |||
const languageClient = await DamlLanguageClient.build( | |||
rootPath, | |||
{}, | |||
process.env, |
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 is why env vars broke
then do | ||
handleCreatedPackageOpenFiles miState home | ||
rebootIdeByHome miState home | ||
else shutdownIdeByHome miState home |
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.
If a daml.yaml containing only an sdk version is created, we shutdown its IDE and don't update any state
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 benefit from being added as a comment to the code. The PR comment below too.
oldIdeData <- atomically $ onSubIde miState home $ \ideData -> (ideData {ideDataIsFullDamlYaml = isFullDamlYaml}, ideData) | ||
if isFullDamlYaml | ||
then do | ||
when (not (ideDataIsFullDamlYaml oldIdeData) && Set.null (ideDataOpenFiles oldIdeData)) $ |
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 want to treat a daml.yaml file going from "only sdk-version" to a full package as though the daml.yaml was created.
We track the previous time the file changed in order to know if it has become valid in this run.
@@ -199,6 +219,12 @@ registerFileWatchersMessage multiPackageHome = | |||
, LSP.FileSystemWatcher (T.pack $ multiPackageHome </> "**/*.dar") Nothing | |||
, LSP.FileSystemWatcher (T.pack $ multiPackageHome </> "**/*.daml") Nothing | |||
] | |||
, LSP.SomeRegistration $ LSP.Registration "MultiIdeOpenFiles" LSP.STextDocumentDidOpen $ LSP.TextDocumentRegistrationOptions $ Just $ LSP.List |
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 was missing from the last PR which made the multi-IDE a little smarter about when to spin up IDEs.
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.
Thanks!
then do | ||
handleCreatedPackageOpenFiles miState home | ||
rebootIdeByHome miState home | ||
else shutdownIdeByHome miState home |
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 benefit from being added as a comment to the code. The PR comment below too.
Fixes to multi-ide for 2.10
Specific fixes:
daml.yaml
files containing only a version, which is common when working with larger projects.We may consider adding
sdk-version
to themulti-package.yaml
, but for now, we add handling for these files(In general, we treat them like they don't exist, but still track information about if they're open, incase they become valid)