You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "michael-hoke (via GitHub)" <gi...@apache.org> on 2023/07/18 14:50:42 UTC
[GitHub] [daffodil-vscode] michael-hoke opened a new pull request, #706: Un-normalize (capitalize) the drive letter from showOpenDialog if we are on Windows
michael-hoke opened a new pull request, #706:
URL: https://github.com/apache/daffodil-vscode/pull/706
Closes #565
--
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] scholarsmate commented on a diff in pull request #706: Un-normalize (capitalize) the drive letter from showOpenDialog if we are on Windows
Posted by "scholarsmate (via GitHub)" <gi...@apache.org>.
scholarsmate commented on code in PR #706:
URL: https://github.com/apache/daffodil-vscode/pull/706#discussion_r1273762728
##########
src/adapter/activateDaffodilDebug.ts:
##########
@@ -52,7 +52,18 @@ async function getFile(fileRequested, label, title) {
})
.then((fileUri) => {
if (fileUri && fileUri[0]) {
- return fileUri[0].fsPath
+ let path = fileUri[0].fsPath
+
+ if (
+ process.platform === 'win32' &&
+ path.charCodeAt(0) > 97 &&
+ path.charCodeAt(0) <= 122 &&
+ path.charAt(1) === ':'
Review Comment:
At this point it's just personal preference aside from the length check which needs to come first and its there now. I'm okay with 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] shanedell merged pull request #706: Un-normalize (capitalize) the drive letter from showOpenDialog if we are on Windows
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell merged PR #706:
URL: https://github.com/apache/daffodil-vscode/pull/706
--
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] scholarsmate commented on a diff in pull request #706: Un-normalize (capitalize) the drive letter from showOpenDialog if we are on Windows
Posted by "scholarsmate (via GitHub)" <gi...@apache.org>.
scholarsmate commented on code in PR #706:
URL: https://github.com/apache/daffodil-vscode/pull/706#discussion_r1271053995
##########
src/adapter/activateDaffodilDebug.ts:
##########
@@ -52,7 +52,18 @@ async function getFile(fileRequested, label, title) {
})
.then((fileUri) => {
if (fileUri && fileUri[0]) {
- return fileUri[0].fsPath
+ let path = fileUri[0].fsPath
+
+ if (
+ process.platform === 'win32' &&
+ path.charCodeAt(0) > 97 &&
+ path.charCodeAt(0) <= 122 &&
+ path.charAt(1) === ':'
Review Comment:
That if statement is nested in another if statement (line 54), so you won't get an NPE. It might be a good idea to ensure that the length of the path is greater than 2 before peeking at different indexes (e.g, `charCodeAt(0)` and `charAt(1)`) without knowing the size of the path. I instinctively look at those fail fast conditionals with what filters things the quickest and order those first. If the second character is not a colon, then it saves you from the next 2 checks.
--
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] michael-hoke commented on a diff in pull request #706: Un-normalize (capitalize) the drive letter from showOpenDialog if we are on Windows
Posted by "michael-hoke (via GitHub)" <gi...@apache.org>.
michael-hoke commented on code in PR #706:
URL: https://github.com/apache/daffodil-vscode/pull/706#discussion_r1271129122
##########
src/adapter/activateDaffodilDebug.ts:
##########
@@ -52,7 +52,18 @@ async function getFile(fileRequested, label, title) {
})
.then((fileUri) => {
if (fileUri && fileUri[0]) {
- return fileUri[0].fsPath
+ let path = fileUri[0].fsPath
+
+ if (
+ process.platform === 'win32' &&
+ path.charCodeAt(0) > 97 &&
+ path.charCodeAt(0) <= 122 &&
+ path.charAt(1) === ':'
Review Comment:
I see what you're saying now - adding the length check is definitely a good idea, but I'm not seeing the reason for moving the `charAt(1) === ':'` above the drive letter check.
The path that makes it here comes from a file dialog, which should mean that it has been validated. For the short circuit to save us from the other checks, that validated path would have to have a lowercase ascii character as the first character and something other than a colon as the second character. Is that any more likely than a validated path that has a non-alphabetical character as the first character and a colon as the second?
--
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] scholarsmate commented on a diff in pull request #706: Un-normalize (capitalize) the drive letter from showOpenDialog if we are on Windows
Posted by "scholarsmate (via GitHub)" <gi...@apache.org>.
scholarsmate commented on code in PR #706:
URL: https://github.com/apache/daffodil-vscode/pull/706#discussion_r1270691668
##########
src/adapter/activateDaffodilDebug.ts:
##########
@@ -52,7 +52,18 @@ async function getFile(fileRequested, label, title) {
})
.then((fileUri) => {
if (fileUri && fileUri[0]) {
- return fileUri[0].fsPath
+ let path = fileUri[0].fsPath
+
+ if (
+ process.platform === 'win32' &&
+ path.charCodeAt(0) > 97 &&
+ path.charCodeAt(0) <= 122 &&
+ path.charAt(1) === ':'
Review Comment:
I'd move this check up 2 lines.
--
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] michael-hoke commented on a diff in pull request #706: Un-normalize (capitalize) the drive letter from showOpenDialog if we are on Windows
Posted by "michael-hoke (via GitHub)" <gi...@apache.org>.
michael-hoke commented on code in PR #706:
URL: https://github.com/apache/daffodil-vscode/pull/706#discussion_r1270775800
##########
src/adapter/activateDaffodilDebug.ts:
##########
@@ -52,7 +52,18 @@ async function getFile(fileRequested, label, title) {
})
.then((fileUri) => {
if (fileUri && fileUri[0]) {
- return fileUri[0].fsPath
+ let path = fileUri[0].fsPath
+
+ if (
+ process.platform === 'win32' &&
+ path.charCodeAt(0) > 97 &&
+ path.charCodeAt(0) <= 122 &&
+ path.charAt(1) === ':'
Review Comment:
Wouldn't that open us up to a NullPointerException? If fileUri[0] is null, it'll get caught by the if, and this code won't be hit.
--
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