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

Allow single-line array and object declarations, and multi-line function declarations #5830

Merged
merged 16 commits into from
Jun 9, 2022

Conversation

anthony-c-martin
Copy link
Member

@anthony-c-martin anthony-c-martin commented Feb 1, 2022

See discussion in #146
Closes #4993
Closes #498
Closes #586

This PR changes the following:

  • Allow functions to be split across multiple lines:
    var foo = resourceGroup(
      mySubscription,
      myRgName)
  • Allow single-line object & array declarations (when used with commas):
    var foo = ['abc', 'def']
    var bar = {abc: 'def', ghi: 'jkl'}
  • Allow mixing and matching of the above:
    var foo = ['abc', 'def'
      'ghi']
    var bar = {a: 'abc', b: 'def'
      c: 'ghi'}
    var baz = concat('foo', 'bar',
      'baz')
  • Adds a code fix to switch between single- and multi-line declarations.

See Installing Nightly if you're interested in trying this out.

Here's a demo of this during the last community call: https://youtu.be/R6SyuRhTmmc?t=1145s

@anthony-c-martin anthony-c-martin changed the title [WIP] Allow splitting functions over multiple lines [WIP] Experimenting with newline sensitivity changes Feb 7, 2022
@antsok
Copy link

antsok commented May 28, 2022

Good stuff! Optional commas in arrays are great.
It can be used in decorators too, right ? @allowed(['slow', 'fast'])

:shipit: please 🥇

@anthony-c-martin
Copy link
Member Author

Good stuff! Optional commas in arrays are great. It can be used in decorators too, right ? @allowed(['slow', 'fast'])

:shipit: please 🥇

Yes it would work in decorators too

@anthony-c-martin
Copy link
Member Author

So I think the options here are:

  1. Reject the implementation
  2. Accept the implementation as-is
  3. Accept the implementation, just for functions (no objects & arrays)
  4. Accept the implementation, but block commas on multiline objects & arrays

I'm leaning most heavily towards option 4 - it's purely additive, doesn't introduce 2 different ways of multiline arrays/objects, but still gives us the option to introduce this in future if we want to.

@anthony-c-martin
Copy link
Member Author

anthony-c-martin commented Jun 6, 2022

I'm leaning most heavily towards option 4 - it's purely additive, doesn't introduce 2 different ways of multiline arrays/objects, but still gives us the option to introduce this in future if we want to.

We had a discussion on this today, and reached consensus on option 4, with code fixes to switch between single- and multi-line declarations. It leaves the door open to doing option 2 in the future if we get community feedback for it; whereas option 2 would be an irreversible decision.

The remaining open question was regarding the formatter. I've proposed just fixing indention and spacing without deciding whether a declaration should or shouldn't be multi-line. We had agreement on wanting to pick a sensible opinionated default, and wanted to discuss whether we should pick a more opinionated Prettier-like approach. Personally I feel like we're unblocked on this PR (with a basic formatter implementation), and can try to get feedback on a community call in the future on a more sophisticated auto-formatter.

@anthony-c-martin anthony-c-martin changed the title [WIP] Experimenting with newline sensitivity changes Allow single-line array and object declarations, and multi-line function declarations Jun 7, 2022
Copy link
Contributor

@shenglol shenglol left a comment

Choose a reason for hiding this comment

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

Looks good! Just a minor suggestion.

@anthony-c-martin anthony-c-martin enabled auto-merge (squash) June 9, 2022 02:35
@anthony-c-martin anthony-c-martin enabled auto-merge (squash) June 9, 2022 02:36
@anthony-c-martin anthony-c-martin merged commit 406bebd into main Jun 9, 2022
@anthony-c-martin anthony-c-martin deleted the ant/sensitivity branch June 9, 2022 04:10
@brwilkinson
Copy link
Collaborator

Thank you for these improvements ✅

A few things, with some comments.

  • Perhaps 1 or 2 of these may be interesting.
  • Also some use cases, that I did not expect, however appear to be fine.

Before

image

  1. "convert object to single line" code action
  • not what I was expecting, however works fine

image

  1. convert object to multiple lines
  • looks good, it's just not indented.

image

then format

  • now back to normal, with indenting

image

looks like convert to single line also puts different property values on new lines

image

it appears to build just fine

image

  • this all seems to work well, in these use cases.
    image

error messages

  • I guess we can improve some messaging around the change i.e. I have to delete the comma.
    image
    image
    image

  • then format, works well
    image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants