You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "Shanedell (via GitHub)" <gi...@apache.org> on 2023/05/05 23:31:58 UTC
[GitHub] [daffodil-vscode] Shanedell opened a new pull request, #626: Keep terminal open after debug ends
Shanedell opened a new pull request, #626:
URL: https://github.com/apache/daffodil-vscode/pull/626
Keep terminal open after debug ends
- Grab active terminal if its a proper shell that can run a command, otherwise create a new one.
- Send command to run debugger to terminal.
- This allows for the terminal to stay open after the debugger exists.
Closes #614
@scholarsmate @mbeckerle This is the better fix for the issue Mike was seeing. This PR will have the code grab active terminal if it can run a new command, otherwise create a new one. Doing it this way then sending the command to run the debugger to the terminal allows for the terminal to stay open after the debugger exists.
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187701579
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
So no we actually don't have to expect any shell like I am currently doing. All that was actually doing is setting the name of the terminal so it can be all removed and we just set the name to `daffodil-debugger`. The VSCode api actually handles that on its own and when we don't specify a script to run it grabs whatever shell is the default for that user.
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187632372
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
Ah I see what you are seeing I will look into adding this. I believe it normally defaults to the shell or command being ran but will see if I could set to `Daffodil` or `daffodil-debugger`.
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187621671
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
Only put in what you are going to create tests for. If we only have tests for cmd and bash, then that's the only thing we should allow.
--
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 pull request #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#issuecomment-1538473662
@scholarsmate @mbeckerle This is the better fix for the issue Mike was seeing. This PR will have the code grab active terminal if it can run a new command, otherwise create a new one. Doing it this way then sending the command to run the debugger to the terminal allows for the terminal to stay open after the debugger exists.
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187617385
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
We possibly could support allow them. Should I just make in `bash` and `cmd`? `cmd` is used for windows and then `bash` is linux/mac
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187630394
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
Well, what value add does it provide?
That people can type commands at the terminal using their preferred shell?
If this is an expected thing under VSCode, i.e, that you can open a terminal in your preferred shell, then maybe we shouldn't be using the regular terminal window for our needs.
Why wouldn't we give them a separate terminal for their command line interactions, and the one we're controlling can be exactly what we want it to be and that we test. (Maybe it can be named Daffodil shell instead of "Terminal" even?) I don't know what's possible, but to me if we're issuing commands that have to work, and they require a terminal window to capture the output, then we need to constrain what shell is used, and that is in tension with a terminal window the user interacts with.
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187623181
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
Do you think there should possibly be a `TODO` comment to add support for others?
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187622231
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
gotcha will fix that now
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187680233
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
@mbeckerle So actually the new terminal just opens default shell. So do you want the name of the terminal to be `daffodil` or `daffodil-debugger`? Will still try to use an open terminal if it has this name otherwise create a new one
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187693329
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
Will this be broken if a VSCode user sets the default shell to something we're not expecting?
If so, then can we stipulate exactly which shell is used in a terminal we create?
--
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 #626: Keep terminal open after debug ends
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell merged PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626
--
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 diff in pull request #626: Keep terminal open after debug ends
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #626:
URL: https://github.com/apache/daffodil-vscode/pull/626#discussion_r1187603722
##########
src/utils.ts:
##########
@@ -26,6 +26,18 @@ import { TDMLConfig } from './adapter/activateDaffodilDebug'
const defaultConf = vscode.workspace.getConfiguration()
let currentConfig: vscode.DebugConfiguration
+// if active termainl is one of these, use the active terminal instead of making a new one
+const allowedShells = [
Review Comment:
Hmmm. Why allow all these shells? Are we really going to attempt to support (and test!) all of these?
My preference would be to put in ONLY entries that we have specific testing of them. Comment the others out and make a note there that these could be supported, but would need testing to cover them.
--
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