Add a codefix for BCP035

[pakrym]

Fixes: #4563

[majastrz]

ToTextPreserveFormatting

In \src\Bicep.Core.IntegrationTests\ParserTests.cs, we have FilesShouldRoundTripSuccessfully() and Oneliners_ShouldRoundTripSuccessfully(). Can you update them to also assert that the result from ToTextPreserveFormatting() matches what the tests get from their own print visitor?

[pakrym]

I was trying to remove the last usage of PrintVisitor and stumbled on this test code in PrintProgram_AnyProgram_ShouldRoundTrip:

			printVisitor.Visit(program);
            string programText = buffer.ToString();

            buffer.Clear();
            printVisitor.Visit(program);
            string formattedProgramText = buffer.ToString();

            formattedProgramText.Should().Be(programText);

Am I missing something or does it compare two equivalent outputs of printVisitor?

[pakrym]

I think it should've been:

            printVisitor.Visit(formattedProgram);
            string formattedProgramText = buffer.ToString();

[pakrym]

Ok, I won't try to fix it in this PR, but FYI it starts failing if you try to format the formattedProgram

[majastrz]

This might be a good question for @shenglol who did the formatter.

[shenglol]

Ahh good catch. That's definitely a bug. I've created #4605 to track it and will try to fix it later.

[pakrym]

@Shengol I ended up fixing the test in this PR (https://github.com/Azure/bicep/pull/4570/files/90f961f9042ae2e8ed9b46712c9172d33d0c6cd8#diff-026bc8f7b01e7149a98805711f52ea0f2df9cc6f0664ad061d6f982358cbe5d4) to get rid of test PrintVisitor.

[shenglol]

Awesome! Thanks for fixing it. I've assigned #4605 to you and linked it.

[majastrz]

If you start with this Bicep file (don't include the | char):

resource foo 'Microsoft.Storage/storageAccounts@2021-04-01' = {
  |
}

And then leave the cursor at the | and then click the codefix, you get the following result:

resource foo 'Microsoft.Storage/storageAccounts@2021-04-01' = {
  
  kind:
  location:
  name:
  sku:
}

Would it be possible to get rid of the extra line above the kind property?

[majastrz]

I added some comments, but this is such a great feature! Thanks for contributing this! ❤

[majastrz]

Btw, there's another case with discriminated object types. The codefix doesn't appear for a resource like this:

resource bar 'Microsoft.Resources/deploymentScripts@2020-10-01' = {
  
}

This is a rarer case, so we don't need to block this PR on this part. The experience here would have to be different because there are n options for required properties. I think we'd need a separate code fix for each possible value of the discriminator property (similar to how we generate the required property snippets for the same).

[majastrz]

The other thing I noticed is that the code fix only adds one level of required properties. For example with modules when I start with an empty body I get this:

module mod 'passthrough.bicep' = {
  name:
  params:
}

Then I have to specify the curly braces and click the code fix again to get the required params as well.

Should we make it recursive to match the snippets?

[majastrz]

Found an indentation bug as well. If I start with:

module mod 'passthrough.bicep' = {
  name: 2
  params: {}
}

Then click the code fix on params, I get this:

module mod 'passthrough.bicep' = {
  name: 2
  params: {
  myInput:
}
}

[pakrym]

I've added more tests and improved indentation.

Should we make it recursive to match the snippets?

Happy to look into it but would prefer to do it in a separate PR.

[pakrym]

CI failure seems to be the build flakiness.

[pakrym]

Makes it so the test server uses the same set of type definitions (AzTypes) as the normal bicep invocation.

[pakrym]

Not sure if my change caused the ModifyBicepConfigFileAndVerifyCallsToFileResolver failure.

[shenglol]

Not sure if my change caused the ModifyBicepConfigFileAndVerifyCallsToFileResolver failure.

Nope that's a flaky test we are working on fixing.

[pakrym]

I think I've addressed all comments in this PR and the OSx failure is build flakiness. Please tell me if there is something else I've missed.

[shenglol]

Looks good to me! I just requeued the build jobs and hopefully they will pass this time.

[majastrz]

LGTM too. Thanks for adding this!