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

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

Conversation

bhsubra
Copy link
Contributor

@bhsubra bhsubra commented May 3, 2021

Changes to return placeholder text as IdentifierName instead of <error> for snippet templates with placeholder text for symbolic name

Snippet templates available here: https://github.com/Azure/bicep/tree/main/src/Bicep.LangServer/Snippets/Templates have placeholder text for symbolic name. We seem to return <error> as IdentiferName while parsing. We should be returning placeholder text instead. Consider below example:
resource ${1:dnsZone} 'Microsoft.Network/dnsZones@2018-05-01' = {
name: ${2:'name'}
location: 'global'
}

We should return ${1:dnsZone} as IdentifierName in this case

@bhsubra bhsubra requested a review from majastrz May 3, 2021 16:51
@majastrz
Copy link
Member

majastrz commented May 4, 2021

Returning <error> for a malformed identifier is the expected behavior in a Bicep file. I'm very hesitant to relax any validation in the compiler pipeline just for snippets.

Can you tell me more why returning <error> for the IdentifierName is a problem for snippets?

@@ -41,14 +46,53 @@ public string IdentifierName
return identifier.Text;

case SkippedTriviaSyntax skipped:
return skipped.Elements.Any() ? LanguageConstants.ErrorName : LanguageConstants.MissingName;

ImmutableArray<SyntaxBase> elements = skipped.Elements;
Copy link
Member

Choose a reason for hiding this comment

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

skipped

This relies on a specific parser recovery behavior, which makes it kind of fragile. (We also want to avoid regexes in the main compiler path.)

If we agree that we need to support it in the parser (see my other comment), it should be its own type of syntax node and parsed by the parser like everything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Marcin, I'll investigate separate syntax node option and see how it goes.

@bhsubra
Copy link
Contributor Author

bhsubra commented May 4, 2021

Returning <error> for a malformed identifier is the expected behavior in a Bicep file. I'm very hesitant to relax any validation in the compiler pipeline just for snippets.

Can you tell me more why returning <error> for the IdentifierName is a problem for snippets?

Having a malformed identifier makes it difficult to figure out resource dependencies in snippet templates with multiple resources. I am wondering if we should skip having placeholder for symbolic name of parent resource in templates with multiple resources.
E.g:

// Automation Module
resource ${1:automationAccountVariable} 'Microsoft.Automation/automationAccounts/modules@2015-10-31' = {
  parent: automationAccount
  name: ${2:'name'}
  properties: {
    contentLink: {
      uri: ${3:'https://content-url.nupkg'}
    }
  }
}

resource automationAccount 'Microsoft.Automation/automationAccounts@2015-10-31' = {
  name: ${4:'name'}
}

@MarcusFelling , for snippet templates with multiple resources, do you think it's okay to skip having placeholder for parent resource symbolic name for now?

@bhsubra
Copy link
Contributor Author

bhsubra commented May 4, 2021

Had an offline discussion with @MarcusFelling. We'll skip adding placeholder for parent resource symbolic name. Made changes to flip ordering of resources to display parent resource first in this PR:#2172

I'll go ahead and close this PR. @majastrz , let me know if you have any concerns.

@bhsubra bhsubra closed this May 4, 2021
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.

Return valid IdentifierName for declarations with placeholder text for symbolic name
2 participants