Skip to content
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

Paste as BicepParams #15897

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Paste as BicepParams #15897

wants to merge 5 commits into from

Conversation

miqm
Copy link
Collaborator

@miqm miqm commented Dec 16, 2024

Fixes #15769

Changes

  • Pasting json values in bicepparam is converted to bicep native object
  • Pasting entire json parameters file is being decompiled into param statements
    • if using directive is already there - it's not being added for the second time
    • overlapping param will be duplicated though

Contributing a feature

  • I have opened a new issue for the proposal, or commented on an existing one, and ensured that the Bicep maintainers are good with the design of the feature being implemented
  • I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
  • I have appropriate test coverage of my new feature

@@ -8,3 +8,8 @@ public record DecompileOptions(
bool AllowMissingParamsAndVarsInNestedTemplates = false,
bool IgnoreTrailingInput = true
);

public record DecompileParamOptions(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I move it into separate file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, but whatever :-). Less of an issue nowadays with Intellisense.

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 9.37%. Comparing base (108d816) to head (5805c71).
Report is 1649 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (108d816) and HEAD (5805c71). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (108d816) HEAD (5805c71)
dotnet 4 0
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #15897       +/-   ##
===========================================
- Coverage   94.28%    9.37%   -84.91%     
===========================================
  Files        1113        7     -1106     
  Lines      100791      288   -100503     
  Branches     8734      123     -8611     
===========================================
- Hits        95028       27    -95001     
+ Misses       4595      261     -4334     
+ Partials     1168        0     -1168     
Flag Coverage Δ
dotnet ?
typescript 9.37% <ø> (+1.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1104 files with indirect coverage changes

@@ -144,6 +146,7 @@ export class PasteAsBicepCommand implements Command {
rangeLength,
jsonContent,
queryCanPaste,
languageId
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add languageId to telemetry?

@StephenWeatherford
Copy link
Contributor

@miqm Been OOF, I can take a look tomorrow.

}

private ProgramSyntax DecompileParametersFile(string jsonInput, Uri entryBicepparamUri, Uri? bicepFileUri)
private ProgramSyntax DecompileParametersFile(string jsonInput, Uri entryBicepparamUri, Uri? bicepFileUri, DecompileParamOptions options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

public const string PasteType_JsonValue = "jsonValue"; // Single JSON value (number, object, array etc)
public const string PasteType_BicepValue = "bicepValue"; // JSON value that is also valid Bicep (e.g. "[1, {}]")

public enum PasteContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use these enums from BicepDecompileForPasteCommandHandler.cs, don't duplicate them

public const string PasteType_BicepValue = "bicepValue"; // JSON value that is also valid Bicep (e.g. "[1, {}]")

public enum PasteContext
private enum PasteType
{
None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're sharing these values with .ts code, please use the form None = 0, String = 1, etc. to be more explicit about their values.


if (metadata is { })
if (metadata is not null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've seen others change "is not null" to "is {}" :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(no suggestions, just think it's funny)

@@ -43,6 +43,11 @@ public enum PasteType
JsonValue,
BicepValue,
}
public enum PasteContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't duplicate these two enums

@StephenWeatherford
Copy link
Contributor

I've left some commets, not done yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paste as bicep in bicepparam files doesn't work
3 participants