-
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
Changes from 3 commits
23a501e
1897524
010e484
31d2793
3d733ca
c441042
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,64 @@ | ||
import {CliWrapper} from "../cli/CliWrapper"; | ||
import {extensions, Uri} from "vscode"; | ||
import path from "node:path"; | ||
import {workspace, RelativePattern, GlobPattern, Disposable} from "vscode"; | ||
import YAML from "yaml"; | ||
|
||
export async function generateBundleSchema(cli: CliWrapper) { | ||
export async function generateBundleSchema( | ||
cli: CliWrapper | ||
): Promise<Disposable> { | ||
// get freshly generated bundle schema | ||
const bundleSchema = await cli.getBundleSchema(); | ||
|
||
// URI scheme for DABs JSON schemas | ||
const dabsUriScheme = "dabs"; | ||
|
||
// URI for bundle root config json schema | ||
const rootConfigSchemaUri = `${dabsUriScheme}:///root.json`; | ||
const rootConfigSchemaUri = `${dabsUriScheme}:///dabs.json`; | ||
|
||
const folder = workspace.workspaceFolders?.[0]; | ||
const configFilePattern = new RelativePattern( | ||
folder!, | ||
"{databricks,bundle}.{yml,yaml}" | ||
); | ||
|
||
// 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 commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need an That would be a bit more efficient but I feel that this code is easier to read. We need the watcher on z |
||
await updateFileGlobs(); | ||
}); | ||
|
||
let configFiles = new Set<string>(); | ||
await updateFileGlobs(); | ||
|
||
async function updateFileGlobs() { | ||
const fileGlobs: GlobPattern[] = [configFilePattern]; | ||
|
||
// find all YAML files that are included in the root config file | ||
for (const configFile of await workspace.findFiles(configFilePattern)) { | ||
try { | ||
const fileContents = await workspace.fs.readFile(configFile); | ||
const config = YAML.parse(fileContents.toString()); | ||
if (config.include) { | ||
fileGlobs.push( | ||
...config.include.map( | ||
(g: string) => new RelativePattern(folder!, g) | ||
) | ||
); | ||
} | ||
} catch (e) { | ||
// ignore errors | ||
} | ||
} | ||
|
||
// expand globs to find all config files | ||
const newConfigFiles = new Set<string>(); | ||
for (const glob of fileGlobs) { | ||
for (const file of await workspace.findFiles(glob)) { | ||
newConfigFiles.add(file.path); | ||
} | ||
} | ||
configFiles = newConfigFiles; | ||
} | ||
|
||
const extensionYaml = extensions.getExtension("redhat.vscode-yaml"); | ||
if (extensionYaml) { | ||
|
@@ -21,16 +69,9 @@ export async function generateBundleSchema(cli: CliWrapper) { | |
redHatYamlSchemaApi.registerContributor( | ||
"dabs", | ||
(resource: string) => { | ||
const validFileNames: string[] = [ | ||
"databricks.yml", | ||
"databricks.yaml", | ||
"bundle.yml", | ||
"bundle.yaml", | ||
]; | ||
for (const name of validFileNames) { | ||
if (path.basename(resource) === name) { | ||
return rootConfigSchemaUri; | ||
} | ||
const resourceUri = Uri.parse(resource); | ||
if (configFiles.has(resourceUri.path)) { | ||
return rootConfigSchemaUri; | ||
Comment on lines
+89
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 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. |
||
} | ||
return undefined; | ||
}, | ||
|
@@ -43,4 +84,6 @@ export async function generateBundleSchema(cli: CliWrapper) { | |
} | ||
); | ||
} | ||
|
||
return 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.
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?
https://github.com/databricks/databricks-vscode/blob/main/packages/databricks-vscode/src/extension.ts#L71-L84
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