Allow linebreak/indent before param default

[helixbass]

@GeoffreyBooth this is a small Prettier-related one:

I've gotten basic comment printing working in Prettier so am now starting to go through their JS comment tests to try and match case-by-case. They have tests for comments preceding param default values (on their own lines). While we allow linebreak/indent after = for regular assignments, currently our grammar doesn't allow it for param default assignments. I don't see any reason why it shouldn't be allowed?

This would be a good candidate to get targeted to eg an ast branch if you want to proceed that way?

[vendethiel]

Doesn’t this allow for e.g. (a = ; 5) ->?

[helixbass]

@vendethiel that appears not to work, I guess analogously to how a = ; 5 doesn't work, though didn't dig into why either one doesn't (since if ; simply corresponds to TERMINATOR you'd think it would)

[vendethiel]

The fix for that probably is at work here. Good :)!

[GeoffreyBooth]

Agreed on the ast branch, I’ll get to work on that.

This is terribly ugly syntax:

(
  q =
  a,
  @p =
    b
) -> q

I assume Prettier isn’t going to output anything so atrocious?

But back to the original point, this is invalid JavaScript:

function foo(a; b) {}

JavaScript of course allows linebreaks because it’s whitespace insignificant, but we aren’t. Our linebreaks are equivalent to JS’ semicolons. If a semicolon isn’t allowed here, shouldn’t our linebreak be similarly disallowed?

[helixbass]

@GeoffreyBooth it's not a syntax I've ever desired. The arguments I could see for allowing it are:

• consistency with regular assignment allowed syntax (where we allow linebreaks after = in a place where a JS semicolon would be invalid)
• looking at the Prettier tests which prompted this, I can see how perhaps someone would want to have a descriptive leading comment before a default value. But in fact, you're correct that Prettier considers this ugly, it flattens it out and prints the comment as trailing after the default value (all inline)

[vendethiel]

If a semicolon isn’t allowed here, shouldn’t our linebreak be similarly disallowed?

But we do allow a semicolon here: (a; b = f ooo, bar; c) -> is perfectly valid coffee, just like replacing those ; with a newline is.

[Inve1951]

newlines also mean commas in array/object literals

[
  1
  2
  a: {
    b
    c
  }
]

btw the output for this looks a bit off atm

[GeoffreyBooth]

@helixbass Do you want to resolve conflicts so we can finish this?

After this, what remains before the ast branch can produce a full AST?

[helixbass]

@GeoffreyBooth this one didn't seem clear-cut as to whether there was support/need to allow it?

But ya these have been most of the obvious "smaller chunks" towards AST support. I'm on a project right now so have been a bit less active but have been adding comments support to the Prettier plugin. Preserving herecomment spacing is one more small one that I haven't done. But then at that point it's probably time to start looking at how to land some of the bigger pieces of AST generation functionality that's currently on my prettier branch

Any interest in a call to discuss approaches to moving forward with that? I figured that once I feel like it's "feature complete" on that branch, I could start looking at its current state and trying to carve out individually reviewable chunks. Also not sure about approaches to testing AST generation, as I've effectively been using the Prettier plugin as my point of testing its accuracy/completeness

[GeoffreyBooth]

@helixbass Sorry to not reply sooner.

this one didn’t seem clear-cut as to whether there was support/need to allow it?

Agreed. If it’s not needed for AST output or Prettier support, it’s best to leave it until after we get those features out the door I think.

As for tests, I started working on some AST tests: https://github.com/GeoffreyBooth/coffeescript/blob/ast-tests/test/abstract_syntax_tree.coffee. The method I’ve been using so far has been to take a string of CoffeeScript code, send it through the compiler with ast: yes set to get AST nodes back, and then inspect the nodes that we get to see if they’re as expected. It’s just a bit tedious as there are 67 node types to test for, basing off the classes in nodes.coffee. I’ve started it, but there are many to go. (@zdenko or @vendethiel or @Inve1951 or anyone else who might want to contribute to this effort, this branch is a great low-effort way to do so!)

A call would be great, I’ll contact you privately to set that up. If anyone else is interested in contributing to the AST/Prettier effort, please let me know and I’ll invite you.

[Inve1951]

@GeoffreyBooth looking good, i'll try to find time for it later today / this weekend.

[Inve1951]

Just an update to this conversation:
AST tests have been written and were merged into coffeescript/ast branch in #5097.

[helixbass]

@Inve1951 thanks, I've gotten my prettier branch up-to-date with the latest ast. Just opened #5099, which is the first PR (against ast) that starts to include AST generation support for some specific node types, as discussed I'm using your tests for those node types as a starting point for having "in-house" tests of the generated ASTs

[helixbass]

Reopened as #5278