-
Notifications
You must be signed in to change notification settings - Fork 20
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
Code completion for DABs includes #819
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #819 +/- ##
=======================================
Coverage 79.78% 79.78%
=======================================
Files 47 47
Lines 1766 1766
Branches 366 366
=======================================
Hits 1409 1409
Misses 356 356
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
||
const folder = workspace.workspaceFolders?.[0]; | ||
const configFilePattern = new RelativePattern( | ||
folder!, |
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.
Return if no folder. Failing gracefully is better.
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.
Aren't we guaranteed to always have a workspace folder?
Still, I agree that we should make this code more defensive.
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.
Yep. Just do not like the look of !
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.
Agreed. I'll change it
|
||
// file watcher on all YAML files | ||
const watcher = workspace.createFileSystemWatcher("**/*.{yml,yaml}"); | ||
watcher.onDidChange(async () => { |
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.
Let's use onDidCreate and onDidDelete
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 also need a watcher on root bundle.yaml, so that we can update globs when the root is updated.
IMO we ONLY need the root watcher.
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 need an onDidCreate
watcher for *.yml
and an onDidChange
watcher for {databricks|bundle}.{yaml|yml}
.
That would be a bit more efficient but I feel that this code is easier to read.
We need the watcher on zonDidCreate
in order to cover include patterns like *.yml
.
const resourceUri = Uri.parse(resource); | ||
if (configFiles.has(resourceUri.path)) { | ||
return rootConfigSchemaUri; |
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.
Alternatively, why can't we just call the updateFileGlobs here? It is not a very expensive function (for reasonable bundles) and less compute intensive than watchers.
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 code is executed whenever a file is opened. I'd like to keep that code path as fast as possible.
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 need not be too expensive. All we need to do is match the resourceUri
with all the include paths in the root bundle.yaml.
That is mostly string matching and regex matching for globs. We shouldn't even need filesystem access if we have a single watcher on root bundle.yaml keeping the list of includes updated.
Apparently we can have root config file including another file (a.yml) which in turn includes another file (b.yml). b.yml need not be explicitly included in the base root config. Need to investigate, but putting this on hold for this release. |
Closing in favour of #892 |
Changes
This PR watches bundle config files and dynamically applies YAML code completion to all files included by the config.
Screen.Recording.2023-07-25.at.10.20.44.mov