You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2022/01/10 19:01:25 UTC
[GitHub] [daffodil-vscode] Shanedell opened a new pull request #71: Fix Launch Wizard for Windows
Shanedell opened a new pull request #71:
URL: https://github.com/apache/daffodil-vscode/pull/71
- Update launchWizard.ts to work for Windows
- For this to work as the different paths caused issues, I opted to switch to reading both the javascript and stylesheet files then putting the content into the HTML as this allows for Linux, Mac and Windows to be able to use the Launch Wizard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] Shanedell commented on a change in pull request #71: Fix Launch Wizard for Windows
Posted by GitBox <gi...@apache.org>.
Shanedell commented on a change in pull request #71:
URL: https://github.com/apache/daffodil-vscode/pull/71#discussion_r782546700
##########
File path: src/launchWizard/launchWizard.ts
##########
@@ -52,12 +53,15 @@ async function createUpdateConfigFile(data, updateOrCreate) {
fs.mkdirSync(`${rootPath}/.vscode`)
}
+ const launchPath =
+ os.platform() === 'win32'
Review comment:
So from my testing on a Windows VM, I believe we don't need the osPath. I believe the open text document file function may be converting paths under the hood as I had never had issues with the class paths until trying to load the style and script files for the launch wizard. Everything else worked fine I assume because it may be handled by a lot of the VSCode API functions by default
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] mbeckerle commented on a change in pull request #71: Fix Launch Wizard for Windows
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #71:
URL: https://github.com/apache/daffodil-vscode/pull/71#discussion_r782559087
##########
File path: src/launchWizard/launchWizard.ts
##########
@@ -52,12 +53,15 @@ async function createUpdateConfigFile(data, updateOrCreate) {
fs.mkdirSync(`${rootPath}/.vscode`)
}
+ const launchPath =
+ os.platform() === 'win32'
Review comment:
If the basic libraries are really covering this, and this is likely one of the only places we have to do os conditionalization (on paths anyway) then I think this code needs a comment to that effect.
Reading this code, a naive typescript coder (like me in a year when I've forgotten all this) would just start proliferating these os.platform() === 'win32' checks. We want to avoid that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] Shanedell merged pull request #71: Fix Launch Wizard for Windows
Posted by GitBox <gi...@apache.org>.
Shanedell merged pull request #71:
URL: https://github.com/apache/daffodil-vscode/pull/71
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] mbeckerle commented on a change in pull request #71: Fix Launch Wizard for Windows
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #71:
URL: https://github.com/apache/daffodil-vscode/pull/71#discussion_r782535452
##########
File path: src/launchWizard/launchWizard.ts
##########
@@ -52,12 +53,15 @@ async function createUpdateConfigFile(data, updateOrCreate) {
fs.mkdirSync(`${rootPath}/.vscode`)
}
+ const launchPath =
+ os.platform() === 'win32'
Review comment:
I can hardly believe you'll get away without needing something like this:
```
osPath(`${rootPath}/.vscode/launch.json`)
```
Which buries the os-specific check down where it is not clutter.
We made this mistake of allowing OS conditionalizations to proliferate in the CLI test code in Daffodil, and since we didn't nip it in the bud, there's an open ticket to basically convert hundreds of tests that have inline OS conditionalizations to use an osPath like library and otherwise push this os/path stuff down a level.
##########
File path: src/launchWizard/launchWizard.ts
##########
@@ -280,10 +282,14 @@ class LaunchWizard {
getWebViewContent() {
const scriptUri = vscode.Uri.parse(
this.ctx.asAbsolutePath('./src/launchWizard/launchWizard.js')
- ).with({ scheme: 'vscode-resource' })
+ )
const styleUri = vscode.Uri.parse(
this.ctx.asAbsolutePath('./src/launchWizard/styles.css')
- ).with({ scheme: 'vscode-resource' })
+ )
+ const scriptData = fs.readFileSync(scriptUri.fsPath)
+ const styleData = fs.readFileSync(styleUri.fsPath)
+
+ // const styleUri = this.getPanel().webview.asWebviewUri(stylePath)
Review comment:
Commented out code. I'd say delete or add a comment to the line why you want it retained.
Eg. "// Todo: keep for now. Still thinking about this."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] mbeckerle commented on a change in pull request #71: Fix Launch Wizard for Windows
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #71:
URL: https://github.com/apache/daffodil-vscode/pull/71#discussion_r782557193
##########
File path: src/launchWizard/launchWizard.ts
##########
@@ -52,12 +53,15 @@ async function createUpdateConfigFile(data, updateOrCreate) {
fs.mkdirSync(`${rootPath}/.vscode`)
}
+ const launchPath =
+ os.platform() === 'win32'
Review comment:
Ok. I'm fine with that if the basic libraries already are dealing with this.
It seems odd that this is the only place we need to do OS conditionalization though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] Shanedell commented on a change in pull request #71: Fix Launch Wizard for Windows
Posted by GitBox <gi...@apache.org>.
Shanedell commented on a change in pull request #71:
URL: https://github.com/apache/daffodil-vscode/pull/71#discussion_r782561844
##########
File path: src/launchWizard/launchWizard.ts
##########
@@ -52,12 +53,15 @@ async function createUpdateConfigFile(data, updateOrCreate) {
fs.mkdirSync(`${rootPath}/.vscode`)
}
+ const launchPath =
+ os.platform() === 'win32'
Review comment:
Yeah so from my understanding most things handle this but in this case we are constructing the entire path to our file based on the folder and on Windows it needs an additional `/` at the beginning of the path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org