Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: workspace cache #95

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Jul 18, 2024

This adds a workspace cache for use by the LSP, but also there's a bug fix for another edge case. I'll point out the test and code that fixes the edge case.

opts.maybe_vendor_override.as_ref(),
),
workspace: new_rc(Workspace::new_single(config_folder)),
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bug fix here. Basically, if someone specifies a path like not-deno.json then we shouldn't do workspace discovery because there's no way that this configuration file could be part of the workspace.

Additionally, if someone specifies a deno.jsonc file when there's a deno.json file, the deno.json file would be part of the workspace and not the deno.jsonc, so we should also not do workspace resolution in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, good catch 👍

}

#[test]
fn test_config_not_deno_workspace_member_non_natural_config_file_name() {
Copy link
Member Author

Choose a reason for hiding this comment

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

opts.maybe_vendor_override.as_ref(),
);
let mut final_members = BTreeMap::new();
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted out to function.

Some(deno_json.dir_path().join("vendor"))
} else {
None
}
}
}
Copy link
Member Author

@dsherret dsherret Jul 18, 2024

Choose a reason for hiding this comment

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

Changed this to just accept a deno json instead of ConfigFolders, which was needlessly complex

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@dsherret dsherret merged commit 6264204 into denoland:main Jul 19, 2024
3 checks passed
@dsherret dsherret deleted the feat_workspace_cache branch July 19, 2024 15:28
dsherret added a commit to denoland/deno that referenced this pull request Jul 19, 2024
- Gets rid of WorkspaceMemberContext. It's now `Workspace` and
`WorkspaceDirectory`
- Uses the workspace cache in the lsp

* denoland/deno_config#95
* denoland/deno_config#96
bartlomieju pushed a commit to denoland/deno that referenced this pull request Jul 22, 2024
- Gets rid of WorkspaceMemberContext. It's now `Workspace` and
`WorkspaceDirectory`
- Uses the workspace cache in the lsp

* denoland/deno_config#95
* denoland/deno_config#96
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants