feat: Add commaNewline option for comma placement control [DLH.io]

[swayam-aicg]

Summary

Adds a new commaNewline formatting option to control whether line breaks occur before or after commas in multi-column/table SQL statements.

Motivation

Teams have different style preferences for comma placement in SQL queries. This option allows users to choose between:

-- commaNewline: 'after' (default)
SELECT
    column1,
    column2,
    column3
FROM table

-- commaNewline: 'before'
SELECT
    column1
    , column2
    , column3
FROM table

Leading commas (comma before newline) are preferred by some teams as they make version control diffs cleaner when adding/removing columns and make it easier to comment out lines. While there are teams that prefer newline before the commas to make it consistent with their code structure.

Changes

• Added commaNewline option with values before / after. The Default is after.
• Added the implementation of the option in formatComma method to place comma accordingly.
• Added documentation for the additional option of having commaNewline option.
• Added tests for the comma functionality covering all the potential cases.

Usage

import { format } from 'sql-formatter';

format('SELECT a, b, c FROM t', {
  commaNewline: 'before'
});

---

Contributed by Swayam Shah at DLH.io

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[karlhorky]

@swayam-aicg thanks for the PR.

However, the current option and value names are a bit hard to understand.

Eg. without knowing about the feature, the following config could be interpreted to mean two opposite things:

1. Comma comes "before" the newline
2. Newline comes "before" the comma

commaNewline: 'before'

It would be good to consider more expressive option and value naming, for example:

commaPosition: 'start' | 'end'

This is more clear (and visual) without knowing about the details of the feature.

[nene]

Thanks for the pull request.

This definitely has been a feature that people have requested.

The implementation looks surprisingly simple. Perhaps it indeed is just so simple, but I'd like to see more tests to be convinced.

Added a few other comments about the naming and choice of space v/s no space, but the most important thing really is the question of how does this affect all the other places that have commas.

[swayam-aicg]

Pushed a new fix to how multiple consecutive comments, and comment blocks can be handled around commas for multiple columns/tables for both of the cases of converting trailing commas to leading ones, and the viceversa. Using Block comments inline may not work as it requires more changes to how comments are handled based on how we need to work with comma placing around it.

[nene]

Thanks for all this effort.

Frankly I didn't think that you were going to immediately tackle this moving of the comments & commas depending on the chosen style. In general the formatter currently never changes the order of tokens. And accordingly its architecture doesn't really lean itself to achieving that (as I'm sure you found out).

Because the formatter doesn't really properly understand the grammar it plays it safe and avoids reordering. Same with addition and removal of tokens. (IIRC it used to have an option for automatically adding final semicolon, but even that was removed.)

Currently the implementation of this comma & comments reordering is such that I can't really convince myself of its correctness (well, I already found a bug, so I know it's not). But more importantly I don't really graps what is the overall approach that you have taken. I guess if I took the time, I could figure it out. But it would be nice if you gave some sort of higher-level overview of how you are tackling this problem.

If I can't convince myself that the approach is sound, then I'd rather leave out this whole thing. It's better to not support this conversion between these commaPosition styles, than to support it with bugs. Most of the time people don't need to convert between two styles (it's nice to have support for this, but it's not a must-have).

PS. I'd also suggest leaving this hard problem out of this initial pull request. Like, I'm mostly happy with what you have done here. If you exclude this comments & commas swapping logic, then I think everything else in here could get merged in fairly quickly. You could then continue working on solving this hard problem in a separate pull request, which I suspect might drag on for longer.

[swayam-aicg]

Thanks for all this effort.

Frankly I didn't think that you were going to immediately tackle this moving of the comments & commas depending on the chosen style. In general the formatter currently never changes the order of tokens. And accordingly its architecture doesn't really lean itself to achieving that (as I'm sure you found out).

Because the formatter doesn't really properly understand the grammar it plays it safe and avoids reordering. Same with addition and removal of tokens. (IIRC it used to have an option for automatically adding final semicolon, but even that was removed.)

Currently the implementation of this comma & comments reordering is such that I can't really convince myself of its correctness (well, I already found a bug, so I know it's not). But more importantly I don't really graps what is the overall approach that you have taken. I guess if I took the time, I could figure it out. But it would be nice if you gave some sort of higher-level overview of how you are tackling this problem.

If I can't convince myself that the approach is sound, then I'd rather leave out this whole thing. It's better to not support this conversion between these commaPosition styles, than to support it with bugs. Most of the time people don't need to convert between two styles (it's nice to have support for this, but it's not a must-have).

PS. I'd also suggest leaving this hard problem out of this initial pull request. Like, I'm mostly happy with what you have done here. If you exclude this comments & commas swapping logic, then I think everything else in here could get merged in fairly quickly. You could then continue working on solving this hard problem in a separate pull request, which I suspect might drag on for longer.

My Approach is rather simple when having multiple lines of comments followed by comma in case of converting from leading type to trailing type so when the formatter comes along a comment, it checks if the next non comment node is a commaNode, and if it's a comma node all the comments are added to temporary cache and the commaNode is handled and all the comments are flushed after it in same order.

In case of converting from trailing to leading the same thing is done in the opposite way where the comma sees and adds all the comment nodes following the comma first, and then adds the comma. This makes sure all comments stay in the line they are meant for.

The bugs and issues encountered are majorly for the block comments, as they are managed differently and in cases of using block comments in the same line, the formatter still treats it as block comment doing the formatting for block comments, also for block comments it doesn't check for multiple lines, and perfecting this can surely take time. Due to time constraints I may not be able to give time to fix it and can just have it ignore the special formatting for block comments for now.

Thanks for your thoughts and suggestions.

[swayam-aicg]

Pushed the updated code that just handles commas and line comments to make it clean.