You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mbeckerle (via GitHub)" <gi...@apache.org> on 2023/06/16 16:33:35 UTC
[GitHub] [daffodil-vscode] mbeckerle commented on a diff in pull request #664: Launch Wizard Validation
mbeckerle commented on code in PR #664:
URL: https://github.com/apache/daffodil-vscode/pull/664#discussion_r1232445732
##########
src/daffodilDebugger.ts:
##########
@@ -189,7 +189,14 @@ export async function getDebugger(
// Get daffodilDebugger class paths to be added to the debugger
let daffodilDebugClasspath = ''
+ //check if each classpath still exists
if (config.daffodilDebugClasspath) {
+ config.daffodilDebugClasspath.split(':').forEach((classpath) => {
+ if (!fs.existsSync(classpath)) {
+ throw new Error(`File or directory: ${classpath} doesn't exist`)
+ }
+ })
+
daffodilDebugClasspath = config.daffodilDebugClasspath.includes(
Review Comment:
What does this line do?
Is this setting a default value of the workspace dir as the only content of daffodilDebugClasspath? If so I would not expect this as the default.
##########
src/daffodilDebugger.ts:
##########
@@ -189,7 +189,14 @@ export async function getDebugger(
// Get daffodilDebugger class paths to be added to the debugger
let daffodilDebugClasspath = ''
+ //check if each classpath still exists
if (config.daffodilDebugClasspath) {
+ config.daffodilDebugClasspath.split(':').forEach((classpath) => {
Review Comment:
Naming: classpath should be named "entry" since it is one entry of the daffodilDebugClasspath.
##########
src/launchWizard/launchWizard.js:
##########
@@ -18,12 +18,62 @@
// Retrieve vscode api - Doing this multiple times causes issues with the scripts
const vscode = acquireVsCodeApi()
+// Function to get config index
+function getConfigIndex() {
+ var configSelectionBox = document.getElementById('configSelected')
+ var configSelectedValue =
+ configSelectionBox.options[configSelectionBox.selectedIndex].value
+
+ if (configSelectedValue === 'New Config') {
+ document.getElementById('nameLabel').style =
+ 'margin-top: 10px; visibility: visible;'
+ } else {
+ document.getElementById('nameLabel').style = 'visibility: hidden;'
+ }
+
+ return configSelectedValue === 'New Config'
+ ? -1
+ : configSelectionBox.selectedIndex
+}
+
+// Function get daffodil debug classpath string
+function getDaffodilDebugClasspathString() {
+ let list = document.getElementById('daffodilDebugClasspathTable')
+
+ let daffodilDebugClasspath = ''
+
+ list.childNodes.forEach((childNode) => {
+ let classpath = childNode.textContent
+ .replaceAll(' ', '') // remove any un-needed whitespace
+ .replace('-', '') // remove initial - in front of every item
+ .split('\n')
+ .filter((cp) => cp != '')
+ .join(':')
+
+ if (classpath != '') {
+ daffodilDebugClasspath +=
+ childNode === list.childNodes[list.childNodes.length - 1]
+ ? classpath
+ : classpath + ':'
+ }
+ })
+
+ return daffodilDebugClasspath
+}
+
// Function to call extension to open file picker
function filePicker(id, description) {
+ let extraData = {}
+ if (id === 'daffodilDebugClasspath') {
+ extraData['daffodilDebugClasspath'] = getDaffodilDebugClasspathString()
+ }
+
vscode.postMessage({
Review Comment:
Why is the extension posting a message to itself telling it to do something, which will happen by side effect. Why is that better than just calling the thing? Is this how non-blocking is achieved?
##########
src/launchWizard/launchWizard.ts:
##########
@@ -188,10 +188,9 @@ async function createWizard(ctx: vscode.ExtensionContext) {
panel.dispose()
Review Comment:
Comment about above code that isn't part of this change set.
This is really a suggestion for the future.
The most common thing I want to do with a config is clone one, then update it.
You currently offer create a new one, update an existing one. But I don't see copy one.
Having gone through the tedium of configuring one config for a test I want to use while debugging, for the most part I want to clone it and just change the test data file to be used.
##########
src/launchWizard/launchWizard.js:
##########
@@ -18,12 +18,62 @@
// Retrieve vscode api - Doing this multiple times causes issues with the scripts
const vscode = acquireVsCodeApi()
+// Function to get config index
+function getConfigIndex() {
+ var configSelectionBox = document.getElementById('configSelected')
+ var configSelectedValue =
+ configSelectionBox.options[configSelectionBox.selectedIndex].value
+
+ if (configSelectedValue === 'New Config') {
+ document.getElementById('nameLabel').style =
+ 'margin-top: 10px; visibility: visible;'
+ } else {
+ document.getElementById('nameLabel').style = 'visibility: hidden;'
+ }
+
+ return configSelectedValue === 'New Config'
+ ? -1
+ : configSelectionBox.selectedIndex
+}
+
+// Function get daffodil debug classpath string
+function getDaffodilDebugClasspathString() {
+ let list = document.getElementById('daffodilDebugClasspathTable')
+
+ let daffodilDebugClasspath = ''
+
+ list.childNodes.forEach((childNode) => {
+ let classpath = childNode.textContent
+ .replaceAll(' ', '') // remove any un-needed whitespace
Review Comment:
How do you know it is un-needed whitespace?
I hate having spaces in file/path names, but people do it.
##########
src/launchWizard/launchWizard.js:
##########
@@ -18,12 +18,62 @@
// Retrieve vscode api - Doing this multiple times causes issues with the scripts
const vscode = acquireVsCodeApi()
+// Function to get config index
+function getConfigIndex() {
+ var configSelectionBox = document.getElementById('configSelected')
+ var configSelectedValue =
+ configSelectionBox.options[configSelectionBox.selectedIndex].value
+
+ if (configSelectedValue === 'New Config') {
+ document.getElementById('nameLabel').style =
+ 'margin-top: 10px; visibility: visible;'
+ } else {
+ document.getElementById('nameLabel').style = 'visibility: hidden;'
+ }
+
+ return configSelectedValue === 'New Config'
+ ? -1
+ : configSelectionBox.selectedIndex
+}
+
+// Function get daffodil debug classpath string
+function getDaffodilDebugClasspathString() {
+ let list = document.getElementById('daffodilDebugClasspathTable')
+
+ let daffodilDebugClasspath = ''
+
+ list.childNodes.forEach((childNode) => {
+ let classpath = childNode.textContent
+ .replaceAll(' ', '') // remove any un-needed whitespace
+ .replace('-', '') // remove initial - in front of every item
+ .split('\n')
+ .filter((cp) => cp != '')
+ .join(':')
+
+ if (classpath != '') {
+ daffodilDebugClasspath +=
+ childNode === list.childNodes[list.childNodes.length - 1]
+ ? classpath
+ : classpath + ':'
Review Comment:
I thought you took care of putting the ":" with the join(':') at line 51.
This code is a funny mix of imperative and functional. I suggest go with all functional, and compute the daffodilDebugClasspath from the childNodes without side effects.
##########
src/launchWizard/launchWizard.js:
##########
@@ -18,12 +18,62 @@
// Retrieve vscode api - Doing this multiple times causes issues with the scripts
Review Comment:
Can you explain why there are both launchWizard.js and launchWizard.ts files? I thought vscode was typescript based, but I noticed some time ago that there is also a smaller portion of ".js".
Any short explanation to motivate this would help me better understand the code. Up to you whether that is just a response to this comment, or becomes a comment in the code (somewhere, perhaps not here) or doc.
--
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