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/05/05 23:03:47 UTC
[GitHub] [daffodil-vscode] michael-hoke opened a new pull request, #625: Add conditional defaults to TDML properties
michael-hoke opened a new pull request, #625:
URL: https://github.com/apache/daffodil-vscode/pull/625
This is an untested draft. Unsure if this is the appropriate fix - Creating a draft PR in order to get asynchronous feedback.
Closes #622
--
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 #625: Add conditional defaults to TDML properties
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell merged PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625
--
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] arosien commented on a diff in pull request #625: Add conditional defaults to TDML properties
Posted by "arosien (via GitHub)" <gi...@apache.org>.
arosien commented on code in PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#discussion_r1188136291
##########
src/utils.ts:
##########
@@ -120,12 +130,16 @@ export function getConfig(
tdmlConfig: tdmlConfig
? tdmlConfig
: {
- action: defaultConf.get('tdmlAction', 'none'),
- name: defaultConf.get('tdmlName', '${command:AskforTDMLName}'),
- description: defaultConf.get(
- 'tdmlDescription',
- '${command:AskForTDMLDescription}'
- ),
+ action: tdmlAction,
+ name: tdmlPropsNeeded
+ ? defaultConf.get('tdmlName', '${command:AskforTDMLName}')
+ : defaultConf.get('tdmlName', 'tdmlNamePlaceholder}'),
Review Comment:
I don't think there should be an action of type `none`; if you don't want a TDML action then the TDML config section should be optional. Then there won't be any need to suppress variable interpolations, and if you do want an action then the variables will get interpolated.
--
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 #625: Add conditional defaults to TDML properties
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#issuecomment-1542830428
So can you rebase this on `main` or make a new branch based off of `main` and try just adding the else I mentioned in issue #622 and see if that works? The launch config wizard was updated in a recent PR so the issue you are referring to might be resolved
--
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 #625: Add conditional defaults to TDML properties
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#issuecomment-1548090224
@michael-hoke Can you please rebase off of `main` and add "Closes https://github.com/apache/daffodil-vscode/issues/622" to git commit message. Hopefully CI will run this time
--
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 pull request #625: Add conditional defaults to TDML properties
Posted by "michael-hoke (via GitHub)" <gi...@apache.org>.
michael-hoke commented on PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#issuecomment-1542775749
Ideally, yes, that is how to fix it, but there seems to be separate code paths for running through the command palette versus running from a launch config. If I try to do that in getConfig, it works with the command palette but not with the launch config.
I don't know if we can change the behavior or where the code changes would have to be for the launch config to work, although I haven't had much time to look into it.
--
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 #625: Add conditional defaults to TDML properties
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#discussion_r1187619693
##########
src/utils.ts:
##########
@@ -120,12 +130,16 @@ export function getConfig(
tdmlConfig: tdmlConfig
? tdmlConfig
: {
- action: defaultConf.get('tdmlAction', 'none'),
- name: defaultConf.get('tdmlName', '${command:AskforTDMLName}'),
- description: defaultConf.get(
- 'tdmlDescription',
- '${command:AskForTDMLDescription}'
- ),
+ action: tdmlAction,
+ name: tdmlPropsNeeded
+ ? defaultConf.get('tdmlName', '${command:AskforTDMLName}')
+ : defaultConf.get('tdmlName', 'tdmlNamePlaceholder}'),
Review Comment:
Hmmm. Why create a different text placeholder string?
Also, what is a tdmlName - there are too many kinds of names - the TDML file's name, the testSuite name, the test itself name,... so use a longer more explicit identifier.
Watch out also for "config", as there is a testSuite defaultConfig and each test has a config also, and a testSuite can have multiple named defineConfig children.
Suggest name "tdmlVSCodeConfig" would keep things clearer.
Ideally, this should prompt you for a TDML file name, and then store it here, i.e., be sticky about the selection.
--
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 #625: Add conditional defaults to TDML properties
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#issuecomment-1544621326
@michael-hoke Could you at least rebase this PR on main and delete the line mentioned. I would be fine approving this really anyways
--
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 #625: Add conditional defaults to TDML properties
Posted by "michael-hoke (via GitHub)" <gi...@apache.org>.
michael-hoke commented on code in PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#discussion_r1187634611
##########
src/utils.ts:
##########
@@ -120,12 +130,16 @@ export function getConfig(
tdmlConfig: tdmlConfig
? tdmlConfig
: {
- action: defaultConf.get('tdmlAction', 'none'),
- name: defaultConf.get('tdmlName', '${command:AskforTDMLName}'),
- description: defaultConf.get(
- 'tdmlDescription',
- '${command:AskForTDMLDescription}'
- ),
+ action: tdmlAction,
+ name: tdmlPropsNeeded
+ ? defaultConf.get('tdmlName', '${command:AskforTDMLName}')
+ : defaultConf.get('tdmlName', 'tdmlNamePlaceholder}'),
Review Comment:
> Hmmm. Why create a different text placeholder string?
>
The root issue of #622 is that VSCode is trying to resolve ${command:AskforTDMLName} even in cases where it shouldn't, i.e. when a user runs a launch config with TDMLAction set to 'none'. The idea of this placeholder is to make it so that configs that are created won't use the command as the default value, which would require users to modify the default launch config to make it work for the default use case. On the other hand, if we run into a config with a TDMLAction that isn't none, we want the command to be the default value.
At best, this only reduces user impact for #622, so we're still looking into ways to fix it.
Thanks for the other suggestions - I don't want to try to overcrowd 1.3.0, but these will be helpful when we look to improve upon the TDML functionality in upcoming releases.
--
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 #625: Add conditional defaults to TDML properties
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#issuecomment-1542771760
The main fix that is really needed is a check that if the tdml action is `none` if it is then you set `name, description and path` to empty strings or null. Since that is not currently being done and the items are left being something like `${command:AskForTDMLName}`, those will get ran. If the debugger has a string `${command:.*}` it will always run that command. So those values need to overridden if you wish to not prompt.
--
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 pull request #625: Add conditional defaults to TDML properties
Posted by "michael-hoke (via GitHub)" <gi...@apache.org>.
michael-hoke commented on PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#issuecomment-1544697153
As a note: the default value for TDMLConfig.Action in package.json needs to remain as 'none' even though that isn't a valid action anymore. The VSCode API function we use to get our default configuration (vscode.workspace.getConfiguration()) keys off of this when it gets the default configuration.
--
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 #625: Add conditional defaults to TDML properties
Posted by "Shanedell (via GitHub)" <gi...@apache.org>.
Shanedell commented on code in PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#discussion_r1190326325
##########
src/tests/suite/utils.test.ts:
##########
@@ -35,12 +35,7 @@ suite('Utils Test Suite', () => {
type: 'none',
path: '${workspaceFolder}/infoset.xml',
},
- tdmlConfig: {
- action: 'none',
- name: '${command:AskForTDMLName}',
- description: '${command:AskForTDMLDescription}',
- path: '${command:AskForTDMLPath}',
- },
+ tdmlConfig: null,
Review Comment:
Remove this line. This will break the testing because you specifically don't add the `tdmlConfig` if the tdml props aren't needed.
--
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 pull request #625: Add conditional defaults to TDML properties
Posted by "michael-hoke (via GitHub)" <gi...@apache.org>.
michael-hoke commented on PR #625:
URL: https://github.com/apache/daffodil-vscode/pull/625#issuecomment-1544623659
@Shanedell working on a fix now - assuming everything passes, it will be up soon. As it is right now, it does not fix the issue that it claims to close, so it should not be merged in anyways.
--
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