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

Make the language "less" newline sensitive #146

Closed
alex-frankel opened this issue Aug 6, 2020 · 22 comments
Closed

Make the language "less" newline sensitive #146

alex-frankel opened this issue Aug 6, 2020 · 22 comments
Assignees
Labels
documentation Improvements or additions to documentation investigate

Comments

@alex-frankel
Copy link
Collaborator

0.2 pending discussion with anders

@alex-frankel alex-frankel added this to the v0.2 milestone Aug 17, 2020
@alex-frankel alex-frankel changed the title Auto newline skip Make the language "less" newline sensitive Sep 16, 2020
@majastrz majastrz self-assigned this Nov 4, 2020
@SimonWahlin
Copy link
Collaborator

SimonWahlin commented Nov 6, 2020

Not sure if this is the right place for this comment, feel free to point me to a better Issue.

If this means I can add newline on long lines for readability this would be great!
I have two scenarios in particular:

Long if-statements, these can be hard to read. For example:

var value = myParam == 'value1' ? settingsTable.value1 : myParam == 'value2' ? settingsTable.value2 : {}

This would be much more readable over several lines:

var value = myParam == 'value1' ? settingsTable.value1 : 
            myParam == 'value2' ? settingsTable.value2 : 
            {}

Same thing goes for conditional deploys when they are implemented:

resource logAnalytics 'Microsoft.OperationalInsights/workspaces@2020-03-01-preview' when (condition) = {

vs

resource logAnalytics 'Microsoft.OperationalInsights/workspaces@2020-03-01-preview' 
when (condition) = {

@alex-frankel
Copy link
Collaborator Author

@alex-frankel alex-frankel modified the milestones: v0.3, v0.4 Mar 8, 2021
@anthony-c-martin
Copy link
Member

Also adding #1760 as a related issue

@anthony-c-martin
Copy link
Member

anthony-c-martin commented Apr 15, 2021

I noticed a dirty hack to split functions over multiple lines, using block comments:

var multilineFunction = resourceGroup(/*
*/ mySubscription, /*
*/ myResourceGroup)

Same thing works for ternary, and probably generally for all expressions:

var test2 = thisIsAVeryLongTernary && iWouldLikeToSplitIt ?/*
*/ acrossMultiple :/*
*/ lines

I don't recommend anyone actually use this 😄

@masters3d
Copy link
Contributor

Can the team document these under limitations? Same as multiple line arrays?

@alex-frankel
Copy link
Collaborator Author

@masters3d - We have a known limitations section in our README. Did you see this there? Where else would you want/expect to see this?

@tfitzmac / @mumian FYI

@alex-frankel alex-frankel added the documentation Improvements or additions to documentation label May 3, 2021
@masters3d
Copy link
Contributor

I don’t see documentation that the language is white space sensitive in certain scenarios. It would be great to see all of these listed.

@anthony-c-martin
Copy link
Member

anthony-c-martin commented Feb 1, 2022

I did some experimenting on a branch, and wanted to write up some notes / get some feedback. If you'd like to try this out and give feedback, install the VSCode extension I built here.

Main things I wanted to solve

  • Support for multi-line function calls.
  • Support for single-line array & object declarations.

Assumptions

  1. We must continue to support multiline object & array declarations without commas.
  2. Single-line array & object declarations must have separators between elements.
  3. We should try to avoid introducing multiple ways of declaring the same thing (e.g. supporting both commas & no commas in a multi-line object).
  4. It would be nice (but not necessary) to have the same behavior across function & array/object declarations.

Initial idea

// Single-line declarations use commas to separate elements
var myObject = { abc: 'def', ghi: 'jkl' }
var myArray = [ 'abc', 'def' ]
var myFunc = concat('abc', 'def')
// Multi-line declarations do not use commas to separate elements
var myObject = {
  abc: 'def'
  ghi: 'jkl'
}
var myArray = [
  'abc'
  'def'
]
var myFunc = concat(
  'abc'
  'def'
)
// Also allow mixing and matching
var mixedArray = [
  'abc', 'def'
  'ghi', 'jkl'
]

Thoughts

  • It's nice to have consistency between functions & objects/arrays, but the function declaration with no commas is quirky. Then again, objects/arrays with no commas are also unusual, but we're used to them.
  • When switching between single-line and multi-line declaration, you'd have to remember to remove the comma.

Alternatives

  1. Functions: commas on both single and multi-line. Arrays/objects: commas on single-line, no commas on multi-line:
    // Single-line
    var myObject = { abc: 'def', ghi: 'jkl' }
    var myArray = [ 'abc', 'def' ]
    var myFunc = concat('abc', 'def')
    // Multi-line
    var myObject = {
      abc: 'def'
      ghi: 'jkl'
    }
    var myArray = [
      'abc'
      'def'
    ]
    var myFunc = concat(
      'abc',
      'def'
    )
  2. Functions/arrays/objects: single-line always has commas, multi-line commas are optional:
    // Single-line
    var myObject = { abc: 'def', ghi: 'jkl' }
    var myArray = [ 'abc', 'def' ]
    var myFunc = concat('abc', 'def')
    // Multi-line
    var myObject = {
      abc: 'def'
      ghi: 'jkl'
    }
    // OR
    var myObject = {
      abc: 'def',
      ghi: 'jkl',
    }
    var myArray = [
      'abc'
      'def'
    ]
    // OR
    var myArray = [
      'abc',
      'def',
    ]
    var myFunc = concat(
      'abc'
      'def'
    )
    // OR
    var myFunc = concat(
      'abc',
      'def'
    )

Feedback

I'd love to get some feedback on the above - feel free to respond with your preference on either:

  • Initial proposal.
  • Alternative 1.
  • Alternative 2.
  • Other - any other alternative you'd like to see.

@slavizh
Copy link
Contributor

slavizh commented Feb 1, 2022

@anthony-c-martin
I am more interested in doing something like this

...
resource interactiveQueryRequestsFailAlert 'Microsoft.Insights/scheduledQueryRules@2021-08-01' = {
  name: empty(hdInsightMonitoring.alertRules.interactiveQueryRequestsFail.id) ?
    alertRules.interactiveQueryRequestsFail.id :
    hdInsightMonitoring.alertRules.interactiveQueryRequestsFail.id
  location: location
  kind: 'LogAlert'
  properties: {
    overrideQueryTimeRange: hdInsightMonitoring.alertRules.interactiveQueryRequestsFail.overrideQueryTimeRange == 0 ?
      null :
      'PT${hdInsightMonitoring.alertRules.interactiveQueryRequestsFail.overrideQueryTimeRange}M'
....

and this

...
resource resourceGroupsRes 'Microsoft.Resources/resourceGroups@2021-04-01' existing =
  if (!empty(hdInsightMonitoring.alertsScope.resourceGroup)) {
  name: hdInsightMonitoring.alertsScope.resourceGroup
  scope: subscription(hdInsightMonitoring.alertsScope.subscriptionId)
}
...

As reading becomes hard when when you have conditions with long names for parameters/variables or you have 2 or more conditions nested or you have conditions on resources that are long.

@anthony-c-martin
Copy link
Member

anthony-c-martin commented Feb 1, 2022

@slavizh - I should have started by saying it's not one or the other 😄. Here I was just focusing on functions & objects/arrays - I think the overall issue has been ignored as it's quite nebulous, and wanted to break it down into targeted areas to improve.

Totally agree that the ternary operator a ? b : c, as well as if & for syntaxes could all do with some focus. I'm thinking we should just create individual issues to track the different improvements.

EDIT: Raised #5829 for this

@rynowak
Copy link
Contributor

rynowak commented Feb 1, 2022

I think Go largely gets this right. Semicolons (their separator) are legal to use as a separator in addition to newlines, but are removed by autoformatting. https://go.dev/ref/spec#Semicolons

You could also choose to make commas required for functions and not for the other usages. For example go uses commas (never optional) in expressions, and semicolons in declarations (optional).

@shenglol
Copy link
Contributor

shenglol commented Feb 2, 2022

At first, I prefer Alternative 1, which is what F# has implemented: https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/lists#creating-and-initializing-lists (the difference is that F# uses semicolons as a separators). However, when switching from single line to multiline array/objects, users would have to remove commas, which makes think that the initial idea is better. I also like @rynowak's idea to let the formatter remove commas in multiline declarations.

@CarloKuip
Copy link

As requested in the 2022-01-27 community call.
Multi-line support (or being less sensitive to new lines) is to useful in one scenario I can think of and that's with API management. In API Management you can define a policy as part of an API resource (https://docs.microsoft.com/en-us/azure/templates/microsoft.apimanagement/service/apis/policies?tabs=bicep)

If you want to embed that in your bicep, you'd have to inline a significant piece of xml (not a full xml doc, but a snippet)
Since you can set the type to rawxml or xml that would provide a nice way to trigger the sensitivity.

When this is not support and the xml needs to remain in an external file, there is already support via the loadTextContent('my-xml-file.xml'). To me that's fine, inline would be maybe a bit more intuitive since it's explicit and maybe would open the road to do parameter substitution in the text content.

@anthony-c-martin
Copy link
Member

@CarloKuip - the good news is you can actually already do this today, using Bicep's multi-line strings 😄

For example:

resource apiPolicy 'Microsoft.ApiManagement/service/apis/policies@2021-01-01-preview' = {
  parent: api
  name: 'policy'
  properties: {
    format: 'rawxml'
    value: '''
<policies>
    <inbound>
        <cors>
            <allowed-origins>
                <origin>__ORIGIN__</origin>
            </allowed-origins>
            <allowed-methods>
                <method>*</method>
            </allowed-methods>
            <allowed-headers>
                <header>*</header>
            </allowed-headers>
            <expose-headers>
                <header>*</header>
            </expose-headers>
        </cors>
    </inbound>
    <backend>
        <forward-request />
    </backend>
    <outbound />
    <on-error />
</policies>
'''
  }
}

Docs here.

@stephaniezyen stephaniezyen modified the milestones: v0.5, v0.6 Mar 11, 2022
@stephaniezyen stephaniezyen moved this to Todo in Bicep Mar 24, 2022
@alex-frankel alex-frankel modified the milestones: v0.6, Committed Backlog Mar 25, 2022
@alex-frankel
Copy link
Collaborator Author

Feedback from MVP summit asking if we should introduce more flexibility on our new line sensitivity:
image

@brwilkinson
Copy link
Collaborator

I will add a comment on this one. I don't want to stop other people having options for what they want and personal preference for code style or configuration management style.

One thing that I have come to appreciate over time when you have large configuration documents, having a single configuration item on each line, ensures that it's easier to maintain in version control and also helps to avoid merge conflicts.

I do appreciate how json formatter takes arrays and puts them on new lines by default, unless you minify the json. I didn't appreciate this at first, however auto format in a deterministic fashion (along with the linting rules) is extremely helpful for not committing errors.

  • often you do have a long list and it's tempting to put multiple items on a single line, however having the ability to autoformat is the most important thing for me. Otherwise after working on code or configurations you have added spaces or changes that are not actually configuration changes. The auto format cleans up the code and alignment, so you only check in actual changes, not just changes in how a document is 'manually' formatted.

To summarize: I am not against a new feature, however it would be my preference if having an auto format + linting, would be available as part of the implementation.

In saying this I am also guilty of the following, ... which @anthony-c-martin mentioned further up in this issue #146 (comment)

        networkSecurityGroup: !(contains(sn, 'NSG') && bool(sn.NSG)) ? null : /*
        */ {
          id: contains(sn, 'NSGID') ? sn.NSGID : NSG[index].id
        }

@anthony-c-martin
Copy link
Member

anthony-c-martin commented May 27, 2022

Feedback from MVP summit asking if we should introduce more flexibility on our new line sensitivity: image

And the feedback from today's community call with a demo of an MVP implementation
image

Link to demo: https://youtu.be/R6SyuRhTmmc?t=1145s

🤔🤔🤔

@brwilkinson
Copy link
Collaborator

brwilkinson commented May 27, 2022

"With functions, we keep the command and can now split over multiple lines" ✅
- Really love this.
- Any chance we can do it without commas as well?
e.g.

var func = resourceGroup(
     mySub
     myRGName
)

I wrote this, just this week, so this feature would avoid the mess below

targetResourceId: ep.eptype != 'azure' ? null :  resourceId( EPs[index].subname, /* subscription
                                                             */ EPs[index].rgname, /*  resourcegroup
                                                             */ EPs[index].restype, /* resourcetype e.g. publicip
 resource name */ '${EPs[index].resnameprefix}${ep.prefix}-${EPs[index].org}-${EPs[index].app}-${EPs[index].env}-${ep.name}')

Array and Objects
- The trailing comma on the last item was confusing on the call, however now I realize I like it.
- I like not having to put the trailing comma
- I like being able to have the trailing comma, since I may comment out a line and it's frustrating cleaning up commas, when you are only just testing commenting something out.

e.g. I would expect below

	var arr = [
	    'abc',
	    'def'
	]

	var arr = [ 'abc', 'def' ]
	
	var arr = [
	    'abc',      //<--- this is nice to have, since i just temporarily commented 'def'
	    //'def'
	]

       var arr = [
         'abc'
         'def'         // updating comment to say that I will always be a fan of no commas here.
      ]               
  • I think I like having the delimiter for single line, however it's only what I am familiar with.
  • I think I time I could grow to like the simplicity of doing spaces only, less characters to deal with.
    • so long as it formats well, I am happy.

After rewatching the video, my brain caught up and I think all of this is good ✅ I didn't vote on the call, however I would say yes now.

@jeskew
Copy link
Contributor

jeskew commented May 27, 2022

Would trailing commas be permitted on function calls, as in:

resourceId(
  'Microsoft.Rp/widgets',
  'myWidget',
)

Not sure if any language other than Python permits that syntax, but it can help generate more useful diffs when adding or removing arguments.

@shenglol
Copy link
Contributor

To summarize: I am not against a new feature, however it would be my preference if having an auto format + linting, would be available as part of the implementation.

@brwilkinson I second that. Actually, I think it is a requirement to update the formatter to support alternative layouts, because currently the formatter uses hard-coded rules and always puts an array/object item on a new line. I'd volunteer to update the formatter (time to implement the second part of https://homepages.inf.ed.ac.uk/wadler/papers/prettier/prettier.pdf 🙂).

@brwilkinson
Copy link
Collaborator

brwilkinson commented May 27, 2022

@shenglol 🙂

Part 2, pretty print with alternate patterns, would be so nice, I think the optimal/ideal solution.

The setting for grouping based on chosen width, seems to be the best of both worlds. to align with the single/multiline capability flexibility language additions in this issue.

When work was being done on ARM template JSON, the capability was implemented to allow expressions to be placed over multiple lines. After that addition, the formatter was then disabled around these function (so formatting became manual).

@StephenWeatherford did some work with a UI 'toolstip' feature to show the following. No formatter was implemented, however the preview was available, which shows the nested/grouped format. (slightly different grouping/flattening than in that doc, however similar concept).

I am already thinking of a decorator to override the default width/grouping for the next line expression 😀

image

Adding one extra comment, with the discussions on Bicep language Param Files, it seems whatever if available in the bicep Modules will also be even more significant in the end-user interface, which are the param files formats. e.g. arrays/objects format specifically related to user configuration, which are more dynamic than the Bicep Modules themselves, so this will be most significant there.

@anthony-c-martin
Copy link
Member

Closing this issue out - going forwards, please create separate issues for the specific syntax that you would like relaxed line break support for:

Repository owner moved this from Todo to Done in Bicep Jun 9, 2022
@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
documentation Improvements or additions to documentation investigate
Projects
Archived in project
Development

No branches or pull requests