Scaffold required resource snippet based on the resourceType & apiVersion #2172

bhsubra · 2021-04-08T01:53:46Z

Fixes #804

Provides resource body snippet completion for static templates and swagger

src/Bicep.LangServer/Completions/BicepCompletionProvider.cs

src/Bicep.LangServer/Snippets/SnippetsProvider.cs

majastrz · 2021-04-09T00:35:28Z

src/Bicep.LangServer/Snippets/SnippetsProvider.cs

 using Bicep.LanguageServer.Completions;

 namespace Bicep.LanguageServer.Snippets
 {
    public class SnippetsProvider : ISnippetsProvider
    {
+        private Dictionary<string, string> resourceTypeToBodyMap = new Dictionary<string, string>();


resourceTypeToBodyMap [](start = 43, length = 21)

Is it possible for the dictionaries to be modified concurrently?

And could you add comments to these to explain what they are storing and how that's used?

In reply to: 610234912 [](ancestors = 610234912)

Added comments here:
9b3995e

What about the concurrency question? Can multiple threads be modifying the dictionary at the same time?

Sorry, I missed this. I don't see a problem with resourceTypeToBodyMap and resourceTypeToDependentsMap as they are modified during Initialize(..). Should happen only once. I am not entirely sure of resourceBodySnippetsCache. I updated to use ConcurrentDictionary to be safe. Let me know if you see any issues or think we need locking mechanism instead.

4fc7f62

Sounds good.

majastrz

Added some comments and questions.

alex-frankel · 2021-04-09T15:00:18Z

How do I trigger this? I tried the vsix attached to this build and couldn't figure it out.

MarcusFelling · 2021-04-12T19:42:57Z

I would like to discuss taking this a step further in our next DSL discussion. This PR has the snippet sourced from a static snippet template. How do we make this more dynamic so snippets don't require updating over time with new api-versions and additional resource types? Can the required properties be pulled from the swagger spec to populate this scaffolding?

majastrz · 2021-04-12T20:13:06Z

I would like to discuss taking this a step further in our next DSL discussion. This PR has the snippet sourced from a static snippet template. How do we make this more dynamic so snippets don't require updating over time with new api-versions and additional resource types? Can the required properties be pulled from the swagger spec to populate this scaffolding?

100% agree. Even though our swagger info is incomplete when it comes to required properties, it will be a great improvement regardless. (And it will also generate issues that should help us drive improvements to required properties in swaggers.)

anthony-c-martin · 2021-04-12T20:17:49Z

I would like to discuss taking this a step further in our next DSL discussion. This PR has the snippet sourced from a static snippet template. How do we make this more dynamic so snippets don't require updating over time with new api-versions and additional resource types? Can the required properties be pulled from the swagger spec to populate this scaffolding?

My understanding is that was what #804 was created to address. I'm a bit confused as this PR doesn't seem to be addressing that feature requrest.

@bhsubra would you mind adding a description of what this PR is fixing?

bhsubra · 2021-04-12T20:59:40Z

I would like to discuss taking this a step further in our next DSL discussion. This PR has the snippet sourced from a static snippet template. How do we make this more dynamic so snippets don't require updating over time with new api-versions and additional resource types? Can the required properties be pulled from the swagger spec to populate this scaffolding?

My understanding is that was what #804 was created to address. I'm a bit confused as this PR doesn't seem to be addressing that feature requrest.

@bhsubra would you mind adding a description of what this PR is fixing?

@anthony-c-martin , updated the description. Could you please take a look and let me know what I am missing? Thanks!

bhsubra · 2021-04-12T21:01:19Z

How do I trigger this? I tried the vsix attached to this build and couldn't figure it out.

@alex-frankel , updated description and attached an example gif. Could you please take a look? Thanks!

anthony-c-martin · 2021-04-13T00:43:20Z

@anthony-c-martin , updated the description. Could you please take a look and let me know what I am missing? Thanks!

My understanding is that the feature request is to generate snippets at runtime, purely based off type information (not from user-submitted files).

To give a concrete example (cursor is |), imagine the user has typed the following and requests completions:
resource storage 'Microsoft.Storage/storageAccounts@2021-02-01' = |

We don't have any user-submitted snippets available for this type, so the best we can offer is {} as the completion (empty object).

However, once I've accepted the completion of {}, you can see we get an immediate type error that name, kind, sku and location are required but not supplied:

So in this case, because we know from the type system that these required fields must be supplied, we could instead dynamically generate a snippet that looks like:

{
  name: $1
  kind: $2
  sku: $3
  location: $4
  $0
}

The same applies at different levels of nesting - e.g. if you've got something like:

resource storage 'Microsoft.Storage/storageAccounts@2021-02-01' = {
  name: 'asdf'
  kind: 'BlobStorage'
  location: 'West US'
  sku: |
}

Again, the type system knows that name is a required field for the sku object, so instead of just {}, we could offer:

{
  name: $1
  $0
}

MarcusFelling · 2021-04-13T01:03:59Z

@anthony-c-martin bingo. I think this PR can be merged as it's a step in the right direction and a new PR can be created for the snippet generation functionality?

anthony-c-martin · 2021-04-13T01:18:20Z

@anthony-c-martin bingo. I think this PR can be merged as it's a step in the right direction and a new PR can be created for the snippet generation functionality?

Yup, totally

src/Bicep.LangServer/Snippets/Templates/res-automation-module.bicep

...rver.IntegrationTests/Completions/SnippetTemplates/res-automation-module/main.combined.bicep

majastrz · 2021-04-22T18:59:12Z

When I inserted the KV required properties I got this:

Can you take a look? We have some multi-line snippets that are doing the right things when it comes to indentation and such.

anthony-c-martin · 2021-04-22T20:32:14Z

src/Bicep.LangServer/Snippets/SnippetsProvider.cs

+            return GetResourceBodyCompletionSnippetFromSwaggerSpec(typeSymbol, label);
+        }
+
+        private Snippet GetResourceBodyCompletionSnippetFromSwaggerSpec(TypeSymbol typeSymbol, string label)


[nit] GetResourceBodyCompletionSnippetFromAzTypes ?

Updated here:
4fc7f62

majastrz · 2021-04-22T20:33:56Z

src/Bicep.LangServer/Snippets/SnippetsProvider.cs

+
+                if (typeProperty.TypeReference.Type is ObjectType objectType)
+                {
+                    snippetText += "\t" + typeProperty.Name + ": {\n";


"\t"

I think you will need to keep track of the indent level to help VS code format the nested properties correctly via AdjustIndentation

This is related to one of the screenshots I posted.

Fixed formatting here:
c1e6263

anthony-c-martin · 2021-04-22T20:34:46Z

src/Bicep.LangServer/Snippets/SnippetsProvider.cs

+
+                    foreach (KeyValuePair<string, TypeProperty> kvp in objectType.Properties.OrderBy(x => x.Key))
+                    {
+                        snippetText = GetSnippetText(kvp.Value, snippetText, ref index);


Should this be +=?

Refactored the code here to use StringBuilder instead:
4fc7f62

majastrz · 2021-04-22T20:38:49Z

src/Bicep.LangServer/Snippets/SnippetsProvider.cs

+
+                if (typeProperty.TypeReference.Type is ObjectType objectType)
+                {
+                    snippetText += "\t" + typeProperty.Name + ": {\n";


+=

Nit - could you switch to a StringBuilder to construct these? Each concatenation allocates more strings. On the other hand, they would be very short-lived and not live past gen0, so there probably isn't a huge memory impact anyway.

In #2341, I believe @jfleisher has done the work to construct a well-formatted partial document from Bicep syntax nodes. I wonder if it's worth using what he's done - that way there would be no need to manually format the document via text manipulation (which I'm sure has a bunch of edge cases).

Fixed formatting here:
c1e6263

Switched to use StringBuilder here:
4fc7f62

majastrz · 2021-04-22T20:40:05Z

Do we have any canonical ordering that we should follow when generating the required property snippets? For example should the name property be first? (There might be some ARM code we could borrow that could help here as well.)

majastrz · 2021-04-22T20:41:38Z

From our last team discussion, didn't we want separate completions for {} and "insert object with required properties"?

@alex-frankel?

alex-frankel · 2021-04-26T15:10:41Z

Do we have any canonical ordering that we should follow when generating the required property snippets?

Not everything is applicable, but we should follow the guidance from the quick start contribution guide: https://github.com/Azure/azure-quickstart-templates/blob/master/1-CONTRIBUTION-GUIDE/best-practices.md#resources

alex-frankel · 2021-04-26T15:12:01Z

didn't we want separate completions for {} and "insert object with required properties"?

Yes - I think empty object ({}) should insert just the empty object and a newline. There should be a separate item for scaffolding. I think we might have said insert-required?

…Snippet(..)

…ppets

majastrz · 2021-05-05T01:44:55Z

There is an issue with resource types whose bodies are discriminated object types:

This will require additional discussion on how we fix it, so I'm fine if the fix comes with a different PR.

majastrz · 2021-05-05T01:49:25Z

I have an idea how we can approach it, but we should discuss with the rest of the team tomorrow.

majastrz

This is really cool! Thanks for getting this done!

bhsubra self-assigned this Apr 8, 2021

bhsubra requested a review from majastrz April 8, 2021 02:00

majastrz reviewed Apr 9, 2021

View reviewed changes

src/Bicep.LangServer/Completions/BicepCompletionProvider.cs Outdated Show resolved Hide resolved

majastrz reviewed Apr 9, 2021

View reviewed changes

src/Bicep.LangServer/Snippets/SnippetsProvider.cs Outdated Show resolved Hide resolved

majastrz reviewed Apr 9, 2021

View reviewed changes

majastrz requested changes Apr 9, 2021

View reviewed changes

bhsubra force-pushed the dev/bhsubra/FixForSnippetsIssue804 branch 2 times, most recently from e97046a to 8ce87e0 Compare April 18, 2021 05:14

bhsubra requested a review from anthony-c-martin April 21, 2021 17:36

bhsubra commented Apr 22, 2021

View reviewed changes

src/Bicep.LangServer/Snippets/Templates/res-automation-module.bicep Outdated Show resolved Hide resolved

bhsubra commented Apr 22, 2021

View reviewed changes

...rver.IntegrationTests/Completions/SnippetTemplates/res-automation-module/main.combined.bicep Outdated Show resolved Hide resolved

anthony-c-martin reviewed Apr 22, 2021

View reviewed changes

majastrz reviewed Apr 22, 2021

View reviewed changes

anthony-c-martin reviewed Apr 22, 2021

View reviewed changes

majastrz reviewed Apr 22, 2021

View reviewed changes

bhsubra force-pushed the dev/bhsubra/FixForSnippetsIssue804 branch from c9ad548 to 4509dd1 Compare April 26, 2021 15:02

bhsubra mentioned this pull request May 4, 2021

Fix IdentifierName issue in snippet templates with placeholder for symbolic name #2490

Closed

bhsubra added 13 commits May 4, 2021 16:49

Initial changes to fix issue #804

ecb6dce

Updated GetResourceCompletionSnippet(..) to GetResourceBodyCompletion…

370bf59

…Snippet(..)

Refactored SnippetsProvider

bdeabf9

Cleaned up SnippetsProvider:

6f54c58

Added null check in GetResourceBodyCompletionSnippet(..) with tests

cd4f15e

Rebase from main

28bddc7

Addressed code review comments

edc30b3

Fix some failing tests

51274c6

Addressed some cr comments and refactored code to return separate sni…

fe5723f

…ppets

Formatting changes

10c22d8

Using parent-child syntax and added description

fc9e0d9

Fixed sort order

5a480e3

Moved parent snippet to the beginning of file

43a0fe1

bhsubra force-pushed the dev/bhsubra/FixForSnippetsIssue804 branch from 721c61c to 43a0fe1 Compare May 5, 2021 00:06

majastrz approved these changes May 5, 2021

View reviewed changes

bhsubra merged commit dab7a34 into main May 5, 2021

bhsubra deleted the dev/bhsubra/FixForSnippetsIssue804 branch May 5, 2021 02:46

majastrz mentioned this pull request May 5, 2021

CI builds runtime has regressed recently #2533

Closed

bhsubra mentioned this pull request May 5, 2021

Fix language server integration tests execution time issue #2534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scaffold required resource snippet based on the resourceType & apiVersion #2172

Scaffold required resource snippet based on the resourceType & apiVersion #2172

bhsubra commented Apr 8, 2021 •

edited

Loading

majastrz Apr 9, 2021

majastrz Apr 9, 2021

bhsubra Apr 21, 2021

majastrz Apr 22, 2021

bhsubra Apr 29, 2021

majastrz May 5, 2021

majastrz left a comment

alex-frankel commented Apr 9, 2021

MarcusFelling commented Apr 12, 2021

majastrz commented Apr 12, 2021

anthony-c-martin commented Apr 12, 2021

bhsubra commented Apr 12, 2021 •

edited

Loading

bhsubra commented Apr 12, 2021

anthony-c-martin commented Apr 13, 2021 •

edited

Loading

MarcusFelling commented Apr 13, 2021

anthony-c-martin commented Apr 13, 2021

majastrz commented Apr 22, 2021

anthony-c-martin Apr 22, 2021

bhsubra Apr 29, 2021

majastrz Apr 22, 2021

majastrz Apr 22, 2021

bhsubra May 4, 2021

anthony-c-martin Apr 22, 2021

bhsubra Apr 29, 2021

majastrz Apr 22, 2021

anthony-c-martin Apr 23, 2021

bhsubra May 4, 2021

majastrz commented Apr 22, 2021

majastrz commented Apr 22, 2021

alex-frankel commented Apr 26, 2021

alex-frankel commented Apr 26, 2021

majastrz commented May 5, 2021

majastrz commented May 5, 2021

majastrz left a comment

Scaffold required resource snippet based on the resourceType & apiVersion #2172

Scaffold required resource snippet based on the resourceType & apiVersion #2172

Conversation

bhsubra commented Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majastrz left a comment

Choose a reason for hiding this comment

alex-frankel commented Apr 9, 2021

MarcusFelling commented Apr 12, 2021

majastrz commented Apr 12, 2021

anthony-c-martin commented Apr 12, 2021

bhsubra commented Apr 12, 2021 • edited Loading

bhsubra commented Apr 12, 2021

anthony-c-martin commented Apr 13, 2021 • edited Loading

MarcusFelling commented Apr 13, 2021

anthony-c-martin commented Apr 13, 2021

majastrz commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majastrz commented Apr 22, 2021

majastrz commented Apr 22, 2021

alex-frankel commented Apr 26, 2021

alex-frankel commented Apr 26, 2021

majastrz commented May 5, 2021

majastrz commented May 5, 2021

majastrz left a comment

Choose a reason for hiding this comment

bhsubra commented Apr 8, 2021 •

edited

Loading

bhsubra commented Apr 12, 2021 •

edited

Loading

anthony-c-martin commented Apr 13, 2021 •

edited

Loading