feat: add interpolation for config options #238
Conversation
krvajal
force-pushed
the
linter_exclude_known_mods
branch
from
033173e to
d8e48b9
Compare
There was a problem hiding this comment.
This is awesome work @krvajal, thanks for taking the time to do this.
The asynchronous work is a nice touch.
My comments are mostly focused around the regex approach. I think we might be able to get away with using the solution that the C++ extension implemented. We might even be able to hook in the vscode-cpptools API and not have to deal with any of this but that might be a bit too much for now.
I also had trouble getting the env regex to work, but that might just be me.
Once again very nice work.
|
|
||
| private async getIncludePaths(): Promise<string[]> { | ||
| let includePaths: string[] = await this._config.get('includePaths', []) | ||
| this.loggingService.logInfo(`using include paths "${includePaths}"`) |
There was a problem hiding this comment.
Maybe output dirs one per line? using include paths:\n ${includePaths.join('\r\n')}.
It would also be a good idea to validate that the include paths exist, although it might become computationally expensive if checked every time we open a file.
| const full = mat[0] | ||
| const varname = mat[1] | ||
| const repl = fixPaths(env[normalizeEnvironmentVarname(varname)]) || '' | ||
| subs.set(full, repl) | ||
| } | ||
|
|
||
| const env_re2 = RegExp(`\\$\\{env\\.(${varValueRegexp})\\}`, 'g') | ||
| while ((mat = env_re2.exec(tmpl))) { | ||
| const full = mat[0] | ||
| const varname = mat[1] | ||
| const repl = fixPaths(env[normalizeEnvironmentVarname(varname)]) || '' | ||
| subs.set(full, repl) | ||
| } | ||
|
|
||
| const env_re3 = RegExp(`\\$env\\{(${varValueRegexp})\\}`, 'g') | ||
| while ((mat = env_re3.exec(tmpl))) { | ||
| const full = mat[0] | ||
| const varname = mat[1] | ||
| const repl = fixPaths(env[normalizeEnvironmentVarname(varname)]) || '' | ||
| subs.set(full, repl) | ||
| } | ||
|
|
||
| const penv_re = RegExp(`\\$penv\\{(${varValueRegexp})\\}`, 'g') | ||
| while ((mat = penv_re.exec(tmpl))) { | ||
| const full = mat[0] | ||
| const varname = mat[1] | ||
| const repl = | ||
| fixPaths(process.env[normalizeEnvironmentVarname(varname)] || '') || '' | ||
| subs.set(full, repl) | ||
| } | ||
|
|
||
| const vendor_re = RegExp(`\\$vendor\\{(${varValueRegexp})\\}`, 'g') | ||
| while ((mat = vendor_re.exec(tmpl))) { | ||
| const full = mat[0] | ||
| const varname = mat[1] | ||
| const repl = | ||
| fixPaths(process.env[normalizeEnvironmentVarname(varname)] || '') || '' | ||
| subs.set(full, repl) | ||
| } | ||
|
|
||
| if ( | ||
| vscode.workspace.workspaceFolders && | ||
| vscode.workspace.workspaceFolders.length > 0 | ||
| ) { | ||
| const folder_re = RegExp( | ||
| `\\$\\{workspaceFolder:(${varValueRegexp})\\}`, | ||
| 'g' | ||
| ) | ||
|
|
||
| mat = folder_re.exec(tmpl) | ||
| while (mat) { | ||
| const full = mat[0] | ||
| const folderName = mat[1] | ||
| const f = vscode.workspace.workspaceFolders.find( | ||
| (folder) => | ||
| folder.name.toLocaleLowerCase() === folderName.toLocaleLowerCase() | ||
| ) | ||
| if (f) { | ||
| subs.set(full, f.uri.fsPath) | ||
| } else { | ||
| log.logWarning(`workspace folder ${folderName} not found`) | ||
| } | ||
| mat = folder_re.exec(tmpl) | ||
| } | ||
| } | ||
|
|
||
| const command_re = RegExp(`\\$\\{command:(${varValueRegexp})\\}`, 'g') | ||
| while ((mat = command_re.exec(tmpl))) { | ||
| if (opts.doNotSupportCommands) { | ||
| log.logWarning(`Commands are not supported for string: ${tmpl}`) | ||
| break | ||
| } | ||
| const full = mat[0] | ||
| const command = mat[1] | ||
| if (subs.has(full)) { | ||
| continue // Don't execute commands more than once per string | ||
| } | ||
| try { | ||
| const command_ret = await vscode.commands.executeCommand( | ||
| command, | ||
| opts.vars.workspaceFolder | ||
| ) | ||
| subs.set(full, `${command_ret}`) | ||
| } catch (e) { | ||
| log.logWarning( | ||
| `Exception while executing command ${command} for string: ${tmpl} ${errorToString( | ||
| e | ||
| )}` | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| let final_str = tmpl | ||
| let didReplacement = false | ||
| subs.forEach((value, key) => { | ||
| if (value !== key) { | ||
| final_str = replaceAll(final_str, key, value) | ||
| didReplacement = true | ||
| } | ||
| }) |
There was a problem hiding this comment.
Not sure, what is the purpose of penv and vendor? I couldn't find them listed as a variable type in VS Code https://code.visualstudio.com/docs/editor/variables-reference. Also, I am not sure if we should have env_re3 since it does not make use of a separator, so I don't think it would work.
From some quick testing that I did the env regex does not seem to work for me on Linux. It always returns an empty string.
On a more general note, since we are going down this path of full-on regex I think we might as well add functionality for the rest of the VS Code variables e.g. config, file, fileDirname, fileBasenameNoExtension. input/command is way trickier so I don't think it's worth trying.
A good code example for this is resolveVariables from C/C++ extension. We can almost certainly copy it with minor modifications:
The only thing that this does not do is handle variables without a separator. Of course we could do that either by changing the regex expression or by hadling ${workspaceFolder} separately like the C++ extension does
If done this way we would not rely on getting the last most final_str being the correct one, which is a bit cryptic IMO. This would also halve the number of regex matches performed.
| const var_re = /\$\{(\w+)\}/g | ||
| let mat: RegExpMatchArray | null = null | ||
| while ((mat = var_re.exec(tmpl))) { | ||
| const full = mat[0] | ||
| const key = mat[1] | ||
| if (key !== 'dollar') { | ||
| // Replace dollar sign at the very end of the expanding process | ||
| const repl = repls[key] | ||
| if (!repl) { | ||
| log.logWarning(`Invalid variable reference ${full} in string: ${tmpl}`) | ||
| } else { | ||
| subs.set(full, repl) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't this a bit too generic? It will end up matching everything that contains ${} regardless if it is a VS Code variable.
From my understanding this is the regex that actually matches ${workspaceFolder} and not folder_re since folder_re contains : at the very end.
See previous comment for possible solution.
|
Another thing we should probably add is |
| const ERROR_REGEX: RegExp = | ||
| /^([a-zA-Z]:\\)*([^:]*):([0-9]+):([0-9]+):\s+(.*)\s+.*?\s+(Error|Warning|Fatal Error):\s(.*)$/gm | ||
|
|
||
| const knownModNames = ['mpi'] |
There was a problem hiding this comment.
The errors for the missing mpi.mod are valid and should not be ignored. As in, gfortran does not automatically include the path where mpi.mod is located. Adding path interpolation + glob support should be able to fix that.
People often forget that they need to configure their extension include paths and compiler arguments, similarly to how they wish to compile their code, otherwise linting results will not be very useful and they will be littered with errors.
|
I am closing this in favour of #284 which has just been merged. Overloading During the next few weeks I will be porting some of the changes proposed here into master, async functionality |
TODO