WIP: Extract refactorings

[StephenWeatherford]

Fixes #14535

[github-actions[bot]]

Test this change out locally with the following install scripts (Action run 11148350828)

VSCode

• Mac/Linux

  bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 11148350828
  

• Windows

  iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 11148350828"
  

Azure CLI

• Mac/Linux

  bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 11148350828
  

• Windows

  iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 11148350828"
  

[github-actions[bot]]

Dotnet Test Results

    72 files   -     36      72 suites   - 36   43m 3s :stopwatch: + 9m 23s 11 269 tests +   174  11 269 :white_check_mark: +   174  0 :zzz: ±0  0 :x: ±0  26 315 runs   - 12 538  26 315 :white_check_mark:  - 12 538  0 :zzz: ±0  0 :x: ±0 

Results for commit 898b38f6. ± Comparison against base commit c5e58ed4.

:recycle: This comment has been updated with latest results.

[anthony-c-martin]

Please could you update the title and add a description of the changes? This'll also make it easier for reviewers wanting to test out the UX.

[anthony-c-martin]

There are a few places where an object can't be substituted (module body & resource body AFAIK, but there may be more). We should block it here (cursor == |)

resource namespace 'Microsoft.EventHub/namespaces@2024-01-01' existing = {|
  name: 'askdjfh'
}

[anthony-c-martin]

Another place we should block the action:

resource eventHub 'Microsoft.EventHub/namespaces/eventhubs@2024-01-01' = if (|foo != null) {
  parent: namespace
  name: foo!
}

[anthony-c-martin]

Technically it's not doing anything wrong here, but I'm wondering if we should block extracting a variable reference into a variable, because it's a bit odd:

var test = ''
var test2 = t|est

[anthony-c-martin]

"Extract parameter" generates invalid code here:

var test2 = {}
var test = |test2

[anthony-c-martin]

Thinking about encouraging adoption of custom types, I'm wondering if we should just avoid showing this option:

[StephenWeatherford]

@anthony-c-martin My thoughts on blocking places where either it doesn't make a lot of sense (e.g. creating a new variable for a variable's value) and where it generates invalid code (e.g. variables inside a parameter default value):

My current decision was not to bother trying to protect the user from this. They'll see the compilation error easily enough. Also, there are quite a few places where it would be invalid, so might be a lot of code required. I'm not aware of any scenarios where the result would be invalid and would also not produce a compilation error - but there might be, these would be more important to consider blocking. Finally, we could always add this functionality if users give feedback that they want it.

Another reason I was hesitant to do this was that it can be frustrating when a menu option doesn't appear for no apparent reason. vscode does appear to have the ability to disable a menu item with a reason, though, so I could experiment with that instead of hiding when it's invalid.

A possible solution to the requirement of lots of code required to detect invalid insertions - we could require there be no current compile errors, and then internally make the changes and check for errors. Although that means we'd have to consider performance since creating the code actions happens on every cursor movement in the editor (we can postpone with a resolvable action so might not be a big issue).

[anthony-c-martin]

In this case:

var thing = ''



var test2 = {|
  value: {
    
  }
}

I get the following:

var thing = ''
var newVariable = {
  value: {}
}



var test2 = newVariable

It looks like it appends directly under the previous declaration, which felt a little bit surprising. The behavior which I think would feel most intuitive to me would be prepending, but with a double new-line gap - e.g.:

var thing = ''



var newVariable = {
  value: {}
}

var test2 = newVariable

[StephenWeatherford]

It looks like it appends directly under the previous declaration, which felt a little bit surprising. The behavior which I think would feel most intuitive to me would be prepending, but with a double new-line gap - e.g.:

Definitely also something I wanted feedback on. I'm currently placing it after the previous declaration of the same type (var/param/type), which would be useful in scenarios where you're going through and creating a bunch of new params for resource properties like in the original scenario for the bug. Also question of whether you should add newline before or after.

I'm not positive if this is the correct behavior or what people would expect. It might vary enough to consider a vscode setting.

This is something the other tools I played with did not do, but Bicep is a bit more commonly structured along lines of params then vars then resources...

(And BTW I'm obviously also a perfectionist... :-)

[levimatheri]

Great job @StephenWeatherford! One thing I noticed and was curious about: Extracting a param works nicely when there's params already defines (it adds the new param next to the last param defined). However, if there are no params defined, it inserts the new param above the resource where the value is extracted from. I wonder if it's feasible to insert the new param above the resources? It's probably a bit of a stretch, but from what I've seen, most users will usually put params at the top of the file. EDIT: I see you were referring to a similar sentiment in your previous comment 😄 . I definitely agree it's more intuitive.

[StephenWeatherford]

Great job @StephenWeatherford! One thing I noticed and was curious about: Extracting a param works nicely when there's params already defines (it adds the new param next to the last param defined). However, if there are no params defined, it inserts the new param above the resource where the value is extracted from. I wonder if it's feasible to insert the new param above the resources? It's probably a bit of a stretch, but from what I've seen, most users will usually put params at the top of the file. EDIT: I see you were referring to a similar sentiment in your previous comment 😄 . I definitely agree it's more intuitive.

Thanks, Levi, that could be accomplished. Do you like the behavior of trying to put the new param after the other params? And would you expect a newline above/below? Thx.

[anthony-c-martin]

Another place we should block the action:

resource eventHub 'Microsoft.EventHub/namespaces/eventhubs@2024-01-01' = if (|foo != null) {
  parent: namespace
  name: foo!
}

As discussed on Teams, this one actually can be supported - the if statement is just fussy about requiring parentheses - so would need to generate:

var newVariable = foo != null
resource eventHub 'Microsoft.EventHub/namespaces/eventhubs@2024-01-01' = if (newVariable) {
  parent: namespace
  name: foo!
}

Instead of:

var newVariable = (foo != null)
resource eventHub 'Microsoft.EventHub/namespaces/eventhubs@2024-01-01' = if newVariable {
  parent: namespace
  name: foo!
}

[StephenWeatherford]

• [ ] Add telemetry for which options are used
• [x] Add "preview" to menu items for now
• [ ] Consider blocking some specific scenarios (this could be added to in the future)
• [ ] Add newline before/after new declaration (preferably if not already blank)
• [ ] If no existing params, insert before resources, etc. Probably follow params -> vars -> resources. Not sure about types. After params?

[levimatheri]

Great job @StephenWeatherford! One thing I noticed and was curious about: Extracting a param works nicely when there's params already defines (it adds the new param next to the last param defined). However, if there are no params defined, it inserts the new param above the resource where the value is extracted from. I wonder if it's feasible to insert the new param above the resources? It's probably a bit of a stretch, but from what I've seen, most users will usually put params at the top of the file. EDIT: I see you were referring to a similar sentiment in your previous comment 😄 . I definitely agree it's more intuitive.

Thanks, Levi, that could be accomplished. Do you like the behavior of trying to put the new param after the other params? And would you expect a newline above/below? Thx.

I like the adding a param below the last param defined. My preference would be a newline separating the two (i.e. above the newly extracted param).

[levimatheri]

Not sure about types. After params?

FWIW AVM adds types after outputs: https://github.com/Azure/bicep-registry-modules/blob/032457048e32f131aa9578da896bc0b801ecd0e5/avm/res/cache/redis/main.bicep#L364