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

Decorator syntax #64

Closed
majastrz opened this issue Jul 2, 2020 · 30 comments · Fixed by #1324
Closed

Decorator syntax #64

majastrz opened this issue Jul 2, 2020 · 30 comments · Fixed by #1324
Assignees
Labels
Needs: Author Feedback Awaiting feedback from the author of the issue syntax Related to language syntax
Milestone

Comments

@majastrz
Copy link
Member

majastrz commented Jul 2, 2020

Overview

ARM copy loops support a mode (serial or parallel) as well as a batchSize parameter to control how looping is done. We seem to have agreement that we should use decorators to express a modification of the loop behavior, but we have not formally agreed on the syntax for them.

Decorators are coming up in other places as well (parameters and outputs), but I put those areas of the language aside in this proposal.

Mode

Option 1

We can set serial mode on a resource loop as follows:

@serial
resource myAccounts 'Microsoft.Storage/storageAccounts?2020-01-01' = [for account in accounts: {
  location: account.location
  name: account.name,
  ...
}]

Parallel mode is the default, but it could be optionally expressed using @parallel instead of @serial in the example above.

Option 2

Instead of two different decorators, we could use one @mode decorators like these:

  • @mode('serial')
  • @mode('parallel')

Batch size

Similar to loop mode, batch size could be expressed with something like @batchSize(2)

Combining decorators

Option 1

We could allow one decorator per line like this:

@parallel
@batchSize(42)

Option 2

We could allow combining decorators on a single line using commas:
@parallel, @batchSize(42)

@majastrz majastrz added the syntax Related to language syntax label Jul 2, 2020
@lwang2016
Copy link
Member

Have we decided to go with decorators? Anders still seems hesitant on it.

@alex-frankel
Copy link
Collaborator

Anders is hesitant, but accepts that it may be the least bad option :)

The other place we have a modifier-like syntax would be parameters. Putting a couple examples here for reference:

@minValue(3)
@maxValue(24)
@description("name of the storage account")
parameter storageAccountName int
@minValue(3), @maxValue(24), @description("name of the storage account")
parameter storageAccountName int

alternative with less @'s

@{minValue(3), maxValue(24), description("name of the storage account")}
parameter storageAccountName int

@lwang2016
Copy link
Member

alternative with less @'s

@{minValue(3), maxValue(24), description("name of the storage account")}
parameter storageAccountName int

I like the idea of decorator block to avoid too many @. Maybe support both syntax?
@minValue(3) for single decorator,
@{multiple decorators}

@majastrz
Copy link
Member Author

majastrz commented Jul 2, 2020

Yup, I like the multi-decorator syntax too.

There's also an interesting argument for combining some of the constraints into a single decorator like @length(3,24)

We might need a restriction on complex values in decorators (objects/arrays) with all the syntaxes above. Otherwise, we'll be in curly brace hell again.

@alex-frankel
Copy link
Collaborator

good point - allowedValues will be a challenge I think...

@allowedValues([
  'small'
  'medium'
  'large'
])
parameter size string

@bterlson
Copy link
Member

bterlson commented Jul 2, 2020

FWIW you don't need commas between multiple decorators (assuming they are called with parens).

Perhaps a pipe dream but it would be great if users could compose a length decorator from a provided minLenth and maxLength decorator. This composition could be used to encapsulate common parameter types users have in their problem domain.

@majastrz
Copy link
Member Author

majastrz commented Jul 3, 2020

@alex-frankel Yeah good pt. allowedValues makes that unavoidable. Also a single allowed value can actually be an object 😈 - ARM does JToken.DeepEquals() to compare.

@bterlson Are you thinking the comma-less group would be in that block syntax above or like this?
@length(1,4) @description('I am so descriptive')

Composing user-defined decorators could definitely be interesting once there are more opportunities for decorating things. Currently agreed upon syntax limits what you could do with them (tweak a loop, tweak a parameter and that's basically it), but I suspect the list where they fit will grow.

@alex-frankel
Copy link
Collaborator

Interesting - it feels like we should flag a single allowedValue as an error or warning since it should just be a variable or hardcoded value at that point, right?

@majastrz
Copy link
Member Author

majastrz commented Jul 3, 2020

Sorry my statement was ambiguous. I meant that in an array of allowed values, a single array item can be an object. (Or another array. Or anything really.)

The type checking we will do here will check that allowedValues is an array and that the type of each item in the array is assignable to the parameter type. We could check the length of the array. 0 would definitely be an error, but 1 is a warning at best. If the user is planning to reserve a parameter and add more allowed values in the future, they could very well just specify one for now. Yes, they could achieve the same by adding the parameter later with a default value, but I don't want to choose that path for them.

@alex-frankel
Copy link
Collaborator

Ah, gotcha. I agree with the logic that it should not be an error, and I can see why we may want to hold off from the warning as well. I've also seen text in a darker/faded out color for things like "unused" params and variables, maybe that would be better.

@alex-frankel alex-frankel added this to the v0.2 milestone Aug 11, 2020
@alex-frankel
Copy link
Collaborator

alex-frankel commented Aug 21, 2020

More supporting evidence to making this change: #293

@StephenWeatherford
Copy link
Contributor

Ah, gotcha. I agree with the logic that it should not be an error, and I can see why we may want to hold off from the warning as well. I've also seen text in a darker/faded out color for things like "unused" params and variables, maybe that would be better.

FYI, the easiest way to do that in vscode is to add the vscode.DiagnosticTag.Unnecessary tag to the warning diagnostic, but if you don't want to add warnings, you could also do it via a text decorator. I'd also suggest you use tslint's model for disabling warnings.

@alex-frankel alex-frankel modified the milestones: v0.2, v0.3 Sep 30, 2020
@anthony-c-martin
Copy link
Member

anthony-c-martin commented Dec 14, 2020

Another potential use-case - semantic documentation which can show up on hovers would be pretty neat (especially when working with modules) - e.g. something like the following if you just pretend my comments are actually useful:

@doc 'The prefix to use for the diagnostics storage account'
param diagsAccountPrefix string

@doc 'The diagnostics storage account name'
var diagsAccountName = '${diagsAccountPrefix}${uniqueString(resourceGroup().id)}'

@doc 'The diagnostics storage account'
resource diagsAccount 'Microsoft.Storage/storageAccounts@2019-06-01' = {
  name: diagsAccountName
  ...
}

@majastrz
Copy link
Member Author

Yeah, being able to add descriptions and other text that shows up in hovers would be amazing.

@shenglol
Copy link
Contributor

shenglol commented Dec 17, 2020

Our team had a discussion and we decided to implement the multi-line decorator syntax for 0.3:

@minValue(3)
@maxValue(24)
@description("name of the storage account")
parameter storageAccountName int

Once we make our language less new line sensitive (#146), we'll need to revise the syntax for putting multiple decorators on the same line.

The @doc decorator for semantic documentation is definitely something we would like to have as well, but it's a separate feature which will come after the 0.3 release.

@alex-frankel alex-frankel added the Needs: Author Feedback Awaiting feedback from the author of the issue label Dec 17, 2020
@bveerendrakumar
Copy link

@alex-frankel I liked the multi line syntax. It's readable, close to decorators concept in c#, so it will be natural to developers.

@lr90
Copy link
Contributor

lr90 commented Dec 17, 2020

Maybe to add though will this also support a decorator for reference if we use the decorator @secure or would the reference be an optional param of the @secure decorator?

@alex-frankel
Copy link
Collaborator

@lr90 are you referring to referencing a keyvault secret? This decorator syntax is for the definition of a parameter, not for providing the value of it. So you will still need to reference using the parameters file. Separately, there is a gap for referencing secrets that we will take care of with #1028 (comment)

I think the @secure decorator will not take any arguments

@lr90
Copy link
Contributor

lr90 commented Dec 17, 2020

Yes I was, taken a look a #1028 and understand now. Thanks

@markgar
Copy link

markgar commented Dec 17, 2020

I also like the multi-line syntax. Seems like a good breaking change to include.

@ChristopherGLewis
Copy link
Contributor

good point - allowedValues will be a challenge I think...

@allowedValues([
  'small'
  'medium'
  'large'
])
parameter size string

Alex - I'd love the Array syntax to be more "standard" with commas and declarable on one line.

@allowedValues([
   'small',
   'medium',
   'large'
 ])

and

@allowedValues([ 'small', 'medium', 'large'])

Even w/o the commas, this would help readability:

@allowedValues([ 'small' 'medium' 'large'])

@majastrz
Copy link
Member Author

We are planning to make a change to make the language more newline agnostic that will unblock single-line arrays for example. Although we haven't closed yet on the separator character.

@alex-frankel
Copy link
Collaborator

@ChristopherGLewis - support for single line arrays is tracked with #586 and there is lots of discussion on commas vs no commas in #498

@johndowns
Copy link
Contributor

Personally I would have preferred to go with one of the options that allows for multiple distinct decorators on a single line (at least as an option). Bicep is all about trying to reduce the amount of boilerplate and extra yak-shaving, and it seems more elegant to me to be able to put a bunch of metadata all together than to have to have multiple lines to do it.

However I'm OK with the multiline syntax as an option, and maybe as an initial step before a single-line alternative is available.

@shenglol shenglol self-assigned this Jan 5, 2021
@bmoore-msft
Copy link
Contributor

I’ve been looking at this for a while and still can’t get use to having "decorators" prior to the thing they are decorating in a declarative language… I think the reason is that the decoration needs context, i.e. what is it decorating? Until I know that, it’s distracting from the common scenario of understanding the thing at it's most basic level.

The first thing I want to know, looking at a declarative syntax, is what is being declared – only after that do the properties describing of it have complete meaning. The sku property for example, is a common property but it's value unique to the resourceType. As well, these are not something I need to constantly see or the first thing I want to see, so progressing through the code top/bottom means it’s more difficult to get to the real meaning in the declaration (though code folding might be able to help). IOW the fact that these additional properties could get a bit lengthy, making it that much harder for me to get to the thing that’s relevant -- what’s being declared.

I wonder if one thing that may be driving our thinking is that we're calling them "decorators", which has meaning from prior art, when in practice I think they are just optional (or non-defaulted) properties of the thing being declared. IOW, the minLength property of a parameter is the same concept regardless of whether the value of it is zero (the default) or something the user specifies - and to me that's not any different from it's type. The condition property of a resource, is no different than the location property in it's task of describing the declared thing and neither of those are required. I think what we may really be talking about are optional properties, but some of our other, more common syntax gets in the way… (not that that's not the correct optimization, just thinking out loud).

So I'm trying this:

parameter userName string
@minLength(3)
@maxLength(16)

And part of my thinking goes to a more declarative syntax for looping, e.g.

resource storage 'Microsoft.Storage/storageAccounts@2020-06-01'  
@[copies(numStorageAccounts), mode(serial), batchSize(4)] // I'm defining a resource and how many of that resource I want
@[condtion(createStorage == true)] =
{
    name: storageAccountName
    location: location
    …

}

I think the above is similar to what @lwang2016 mentioned - some variation a block…

And now @johndowns I have to go try to get the image of yak shaving out of my head.

@alex-frankel
Copy link
Collaborator

OK we discussed this today, and I think this is the last time we are going to ask for feedback on this one.

Basically, we're still thinking that decorators will go above the param declaration, but it will be slightly more verbose. This syntax works for both single-line and multi-line, so we only need one syntax:

@{ minLength(3) }
param name string = 'defaultName'
@{
  minLength(3)
  maxLength(24)
}
param name string = 'defaultName'

This also makes folding all of the decorators in vs code easy.

@{...
}
param name string = 'defaultName'

Presumably, when we have support for single-line objects (#586) you could also have:

@{ minLength(3), maxLength(24) }
param name string = 'defaultName'

Thoughts?

If you made it this far, we are also not sure which character we should use after the @. This is more artistic choice, but curious what people like best:

// curly brace
@{...
}

// square brace
@[...
]

// arrows
@<...
>

// others?

@bterlson
Copy link
Member

FWIW I too often like having decorators on the same line for short, e.g. zero-arg decorators like

@fancy param name string = 'fancyName'`

@ChristopherGLewis
Copy link
Contributor

Alex - I like the way this is going, but have a couple of suggestions.

When I'm doing powershell parameters, I always start with the parameter:

[string] $Param

Then as I clean up my code, I add the validators etc.

[ValidateSet(Tom,Dick,Harriet)]
[string] $Param

With the Bicep tooling, it would be great to be able to do the same, starting with the parameter

param name string = 'defaultName'

Then as you make the code better, going to the line above and pressing the @ key would populate the possible decorators based of the parameter type. For Int you would be offered min/max, for string you would get minLength/maxLength (among other possibilities).

@miqm
Copy link
Collaborator

miqm commented Jan 22, 2021

I'd go with square [ or as it's in c# - just @ in each line (no braces at all).

@shenglol
Copy link
Contributor

We discussed this again with Anders, and he prefers the current multiline @decorator syntax. Regarding folding decorators, it is still achievable by implementing the LSP FoldingRangeClientCapabilities.

@shenglol shenglol mentioned this issue Jan 22, 2021
@shenglol shenglol linked a pull request Jan 22, 2021 that will close this issue
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs: Author Feedback Awaiting feedback from the author of the issue syntax Related to language syntax
Projects
None yet
Development

Successfully merging a pull request may close this issue.