Apply various formatting fixes

[joelmukuthu]

Fixes:

• Both proposals in Possible bug [postgres]: line break between function parethesis when indented #74:
  • Break long binary expressions into multiple lines
  • Prevent line break between empty parens, except when there are comments
• Discussion [postgres]: How to format COMMENT statements spanning multiple lines #76
  • Also applies a similar fix to CREATE INDEX statements
• Discussion [postgres]: multi-line WHEN/THEN clauses in CASE statements #78 - with no line-break between multiple WHEN clauses

[joelmukuthu]

Is there a way to avoid handling each case individually here?

[nene]

At the moment I think not much can be done here. This needs some changes on the parser side to make it easier to handle within the prettier plugin.

[nene]

Thanks for the pull request. I've been busy with other stuff. Finally got to reviewing this.

In general it looks good:

• Break long WHEN/THEN clauses into separate lines
  • Needs some more thought on how to actually indent these.
• Break long COMMENT ON clauses into separate lines
  • Doesn't really match with the indentation suggested in #76
• Break long CREATE INDEX clauses into separate lines ✅
• Break long binary expressions into separate lines ✅
• Avoid line-break between empty parenthesis
  • This is the trickiest change. The general logic looks sound. I do have some recommendations though.

[nene]

I find this not so easy to read, because WHEN and THEN blocks are indented the same amount. IMHO it's sort of like formatting if-else in some other language like so:

if x > 10
return 15
else if x < 10
return 20

It also doesn't match with how the procedural version of CASE expression gets formatted:

CASE
  WHEN column_name = 1 THEN
    SELECT \good'
  WHEN columne_name = 2 THEN
    SELECT 'bad'
  ELSE
    SELECT 'other'
END CASE

See case.test.ts

I would go with similar indentation for the long WHEN..THEN blocks in general:

CASE
  WHEN column_name = 1 THEN
    result_name
  WHEN columne_name = 2 THEN
    other_result
  ELSE
    foo
END CASE

[nene]

This indentation doesn't quite match with what was proposed in #76. According to that issue, it really should be:

COMMENT ON
  CONSTRAINT constraint_name ON DOMAIN domain_name
  IS 'This is a really nice comment here.'

Though I admit that the version you implemented might better align with the current overall indentation style that the formatter produces. But on another hand I'm not really so happy with that overall style. I'd like to move it more towards a bit more indentation. For example I'm not too happy with the indentation of ALTER TABLE statements:

-- current
ALTER TABLE foo
ALTER COLUMN bar
SET DEFAULT 'hello';

-- I'd rather have it like:
ALTER TABLE foo
  ALTER COLUMN bar
    SET DEFAULT 'hello';

Anyway... this COMMENT ON formatting is a minor thing. I don't mind if you leave it like this or change it.

[nene]

A better way to implement this, would be:

const hasComments = (node: Node): boolean =>
  (node.leading?.length ?? 0) > 0 || (node.trailing?.length ?? 0) > 0;

The comments field is added to nodes by the Prettier engine. But the leading & trailing fields come directly from our parser.

Given that in here we really are interested from where the comments were in the original source code, we're better off using the leading & trailing. Plus we don't need the type-casts. Additionally it should be more resilient against changes in Prettier.

[nene]

I would turn this function into:

const isEmptyParenExpr = (expr: ParenExpr): boolean => {

Then all that logic for determining whether it's an empty parenthesis would live in one place, and also the name of the function would IMHO be easier to understand.

[joelmukuthu]

Thanks for the review. Everything you write makes sense! I'm going to be AFK for the next 2 weeks so I'll respond/make updates after :)