Preserve the tasks & configuration while adding .net specific ones #2009
Conversation
|
|
gregg-miskelly
left a comment
gregg-miskelly
left a comment
There was a problem hiding this comment.
Otherwise LGTM. Thanks for the submission!
- Gregg
|
|
||
| // Read existing Tasks configuration | ||
| const tasksConfigs = vscode.workspace.getConfiguration('tasks'); | ||
| let existingTaskConfigs = tasksConfigs.get<Array<tasks.TaskDescription>>('tasks'); |
There was a problem hiding this comment.
@vishrutshah thanks for sending this! One question on this - did you test the behavior if you do NOT already have a tasks.json, or if the task.json has no real content to see if you need additional if statements there?
There was a problem hiding this comment.
I've verified with having no task.json at all but let me also verify if tasks.json is having empty content :-)
There was a problem hiding this comment.
Ok. So after testing both the scenarios, looks like we do not need addition if statements around
let existingTaskConfigs = tasksConfigs.get<Array<tasks.TaskDescription>>('tasks');
Case 1: When we do not have tasks.json then getConfiguration returns an object on which we call get<T> which returns undefined
Case 2: When we have tasks.json with empty array of tasks then we do not visit this path as we existing logic resolve({ updateTasksJson: buildTasks.length === 0 }); would prevent from modifying the tasks.json
Let me know your thoughts?
There was a problem hiding this comment.
@vishrutshah your analysis sounds correct to me.
There was a problem hiding this comment.
Cool. Let me know if any more changes are required. Happy to do it :)
src/assets.ts
Outdated
| if (existingLaunchConfigs) { | ||
| launchJson = launchJson.substring(0, launchJson.length-1); | ||
| let existingLaunchConfigsString = JSON.stringify(existingLaunchConfigs, null, ' '); | ||
| existingLaunchConfigsString = existingLaunchConfigsString.substring(1, existingLaunchConfigsString.length-1); |
There was a problem hiding this comment.
In case there is trailing white space, we should probably look for the last index of ']' rather than just assuming it is the very character in the buffer
vishrutshah
force-pushed
the
vscode-banner-bugfix
branch
from
26a6928 to
af67b5b
Compare
|
@gregg-miskelly Could you please help me see what I may doing wrong here as CI is failing? Thanks! |
|
Aha seems like I was missing updates from upstream :-) |
src/assets.ts
Outdated
| let existingLaunchConfigsString = JSON.stringify(existingLaunchConfigs, null, ' '); | ||
| const lastBracket = launchJson.lastIndexOf(']'); | ||
| const lastBracketInExistingConfig = existingLaunchConfigsString.lastIndexOf(']'); | ||
| const firstBracketInExistingConfig = existingLaunchConfigsString.lastIndexOf('['); |
There was a problem hiding this comment.
On this line, don't you want existingLaunchConfigsString.indexOf('['); instead of lastIndexOf?
There was a problem hiding this comment.
Indeed. My mistake. It did work as there was only one occurrence of [. Thanks for catching. Let me fix this
|
Otherwise LGTM. @DustinCampbell @TheRealPiotrP would either of you like to review as well? |
vishrutshah
force-pushed
the
vscode-banner-bugfix
branch
from
b4b08b9 to
57194e2
Compare
gregg-miskelly
left a comment
gregg-miskelly
left a comment
There was a problem hiding this comment.
LGTM. Thanks again!
|
Thanks @gregg-miskelly for helping review the changes! |
4 participants
Fixes #1993