-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adding support for Rascal Project creation PR-redo #248
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 4084b9e.
Kudos, SonarCloud Quality Gate passed! |
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.
I've added review comments. Again: only relevant if we don't try to reuse the rascal function.
@@ -112,7 +117,7 @@ | |||
"devDependencies": { | |||
"@types/glob": "^7.2.0", | |||
"@types/mocha": "^9.1.1", | |||
"@types/node": "^16.x", | |||
"@types/node": "^16.18.23", |
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.
is there a specific reason to pin point this?
@@ -28,13 +28,18 @@ import * as os from 'os'; | |||
import * as path from 'path'; | |||
import * as vscode from 'vscode'; | |||
|
|||
import { writeFileSync } from 'fs'; |
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.
Two concerns:
- we try to avoid "native" parts of node. since those will prevent us from running as a browser extension, if we ever get that far.
- VS Code doesn't like it if we run stuff in
sync
. Since all the extensions in vscode share the same node-process, we have to be good neighours, and write our code in an async way. luckily, with promises this has gotten a lot more readable. - VS Code has a Virtual File System, we should try our best to keep within that, and not escape it more than needed.
import { integer } from 'vscode-languageclient/node'; | ||
import { getJavaExecutable } from './auto-jvm/JavaLookup'; | ||
import { RascalLanguageServer } from './lsp/RascalLanguageServer'; | ||
import { LanguageParameter, ParameterizedLanguageServer } from './lsp/ParameterizedLanguageServer'; | ||
import { RascalTerminalLinkProvider } from './RascalTerminalLinkProvider'; | ||
import { VSCodeUriResolverServer } from './fs/VSCodeURIResolver'; | ||
|
||
const vsfs = vscode.workspace.fs; | ||
const URI = vscode.Uri; |
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.
can't this be done at import time? import { Uri } from 'vscode'
?
@@ -50,6 +55,7 @@ export class RascalExtension implements vscode.Disposable { | |||
this.registerTerminalCommand(); | |||
this.registerMainRun(); | |||
this.registerImportModule(); | |||
this.createProject(); |
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.
nitpick, let's rename it to registerCreateProject
as it's not actually creating a project.
if (filePath) { | ||
const dest = filePath[0].path; | ||
const destUri = URI.file(dest); | ||
const projectName = dest.split("/").at(-1) as string; |
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.
on windows the path won't be seperated by /
. The module path
in node has logic that helps us deal with paths. If we do it on URI paths, we have to use posix
from the path module. The RascalMFValidator
has some example of this.
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.
I do like the idea of forcing the projectName as a directory name in this explicit manner.
We could also use the inverse, and popup some input boxes, and then ask for the root directory.
await vscode.commands.executeCommand("vscode.openFolder", destUri, true); | ||
} | ||
} catch (e) { | ||
console.error(e); |
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.
I think if we fail the command, we should show a message, as otherwise the command has no output, it just didn't do anything observable.
Using the vscode.window.withProgress
will also help give some feedback to what is happening.
|
||
async function newRascalProject(dest: string, name: string) { | ||
|
||
const baseDest = URI.file(dest); |
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.
before calling the function, we already calculated the Uri, we could move this up, so that this can also work for non-file uri's (like virtual file systems).
await vsfs.createDirectory(srcPath); | ||
|
||
const rascalMFPath = URI.joinPath(metaPath, "RASCAL.MF"); | ||
writeFileSync(rascalMFPath.fsPath, rascalMF(name)); |
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.
As discussed earlier, we should do await vsfs.writeFile
(which takes a byte array as argument, so we have to produce the utf8 encoded variant of the string). But that both allows us to work with any virtual file system, and be a better VS Code neighbor.
Hi @maveme I hope you don't mind we continue the discussion of #246 here.
Let's take some time to discuss the tradeoffs properly.
My concerns:
util::Reflective::createRascalProject
that also sets up the correct files. I would prefer reusing this, as that way we don't have to have duplicate knowledge (and bugfixes) of how to correctly generate a project. If you need a pointer to how we call rascal from typescript, take a look at howsendRegisterLanguage
is wired through. I'll be happy to jump on a call and discuss this.