-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(vscode): Add Parameters.json in .Net project conversion #5110
Conversation
- add parameters.json to the csproj file when converting to .net (if it's present in the target directory)
@@ -254,6 +255,10 @@ async function getArtifactNamesFromProject(target: vscode.Uri): Promise<Record<s | |||
continue; | |||
} | |||
|
|||
if (file === parametersFileName) { | |||
artifactDict['connections'].push(parametersFileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find any test coverage for this, am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think there are no tests for this file.
@@ -254,6 +255,10 @@ async function getArtifactNamesFromProject(target: vscode.Uri): Promise<Record<s | |||
continue; | |||
} | |||
|
|||
if (file === parametersFileName) { | |||
artifactDict['connections'].push(parametersFileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using 'connections' not going to cause a key collision in the dictionary overriding where 'connections' is already used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thats right, @mireedmsft You might need to change to 'parameters'. Sorry I went ahead and merge it. Thanks @ynauls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in this case artifactDict is a dictionary of string to array, so adding the parameters file here just adds another element to the array at that index (and updateBuildFile already assumes that each element is an array and iterates through)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this will work, but I agree is a bit wonky.. since this is already merged I'll make another PR and send it out by EOD
@mireedmsft You might need to make some changes in the |
ConnectionParameterization: - add parameters.json to the csproj file when converting to .net (if it's present in the target directory)
* fix(vscode): Add Parameters.json in .Net project conversion (#5110) ConnectionParameterization: - add parameters.json to the csproj file when converting to .net (if it's present in the target directory) * fix(vscode): Refactor Parameters.json into a separate artifact class when converting to NuGet project (#5116) fix(vscode) AddParametersToArtifacts --------- Co-authored-by: Michael Reed <60761072+mireedmsft@users.noreply.github.com>
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix to add parameters.json to the csproj file when converting to a .Net project
What is the current behavior? (You can also link to an open issue here)
parameters.json file is not properly included in the resulting .csproj file, which causes build issues when opened in VSCode or Visual Studio
What is the new behavior (if this is a feature change)?
converted file opens and builds properly in VSCode
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No
Please Include Screenshots or Videos of the intended change:
N/A