Add module path completions for ARM template files #3616
Conversation
shenglol
changed the title
|
Do you know what's going on here? As a test, I copy+pasted a lot of the json files in a particular directory ( Note there's also the fact that space characters are being URL-encoded as |
| if ((PathHelper.HasExtension(fileUri, LanguageConstants.JsonFileExtension) || | ||
| PathHelper.HasExtension(fileUri, LanguageConstants.JsoncFileExtension)) && | ||
| this.FileResolver.TryRead(fileUri, out var fileContents, out var _, Encoding.UTF8, 2000, out var _)) |
There was a problem hiding this comment.
Any perf concerns with this approach? It seems fast enough to me with ~500 files on a new-ish MacBook with an SSD, but could slow down with larger quantities of files or HDDs / other unusual disk configurations.
I get the need for it (as you don't want param files showing up), so not sure if we really have another option!
There was a problem hiding this comment.
Yeah this can definitely be slow with HDDs if there are a hundreds of files in the directory. Although I cannot think of a way to avoid reading the files (as you mentioned we don't want random JSON files showing up in the completion list), we are only checking the first 2000 characters of each JSON file, so the delay of loading completions is linear to the number of files and is not affected by file sizes.
There was a problem hiding this comment.
I suspect we can go lower than 2000. The schema property is usually at the beginning and isn't that long. If the user decides to put it at the end of the long template, I think it's ok if the file doesn't show up in completions. For those cases, the user shouldn't be blocked because they can still write in a valid path.
I think there are a couple of options to preserve the experience if perf ends up being slow for some users:
- We can put in a hard time limiter in the logic. It could be as simple as starting a stopwatch. If elapsed time exceeds some threshold, we stop reading more file and don't return any of the JSON completions.
- We are already receiving notifications about all file events in a workspace (and discarding most of them). We could actually keep track of JSON files specifically and load them only when they are modified to determine if they are a template file. Then the completion logic would just have to query the cache.
| return model.Compilation.SourceFileGrouping.SourceFiles.Any(sourceFile => | ||
| sourceFile is BicepFile && | ||
| sourceFile.FileUri.LocalPath.Equals(fileUri.LocalPath, PathHelper.PathComparison)); |
There was a problem hiding this comment.
Are you trying to account for non-.bicep files which are already part of the compilation? Personally, I think I'd prefer to just keep it purely extension-based, or the completion experience could be confusing. Some examples:
- I've got 2 non-
.bicepfiles, only one referenced in my code. It may be unclear why only one shows up in completions. - File completions may be influenced by whether or not there's a syntax error in the code.
There was a problem hiding this comment.
Good pt. We'd better make the user experience more consistent. I changed it to check .bicep extension only.
| if (model.Compilation.SourceFileGrouping.SourceFiles.Any(sourceFile => | ||
| sourceFile is ArmTemplateFile && | ||
| sourceFile.FileUri.LocalPath.Equals(fileUri.LocalPath, PathHelper.PathComparison))) |
There was a problem hiding this comment.
Is it possible to have ArmTemplateFiles in the source tree without the standard set of extensions? Or are you just trying to avoid having to run the read-from-disk block when possible?
There was a problem hiding this comment.
No, it's not possible. If a referenced file's extension is not within the standard set of extensions (.arm, .json, .jsonc, .bicep), we assume the file to be a bicep file. And yes, this check is done before reading the file to avoid unnecessary I/O.
Good catch 😅. This is indeed a bug. I misunderstood the purpose of the |
| @@ -152,6 +150,39 @@ public bool TryReadAsBase64(Uri fileUri, [NotNullWhen(true)] out string? fileBas | |||
| } | |||
| } | |||
|
|
|||
| public bool TryReadAtMostNCharaters(Uri fileUri, Encoding fileEncoding, int n, [NotNullWhen(true)] out string? fileContents) | |||
Closes #3595.