darold / pgFormatter

A PostgreSQL SQL syntax beautifier that can work as a console program or as a CGI. On-line demo site at http://sqlformat.darold.net/
PostgreSQL License
1.66k stars 100 forks source link

Formatting is not idempotent #321

Closed bbstilson closed 1 year ago

bbstilson commented 1 year ago

First off, I just want to say that this is the best formatter I've used and it's the 4th one I've tried, so thank you for all your efforts.

While implementing this as a CI check, I noticed a case where formatting is not idempotent. That is, formatting the same file twice results in different outputs. If f(x) = formatting(file), then f(f(x)) != f(x)

Here is a minimum reproducible case.

Given this migration:

insert into foo(id) values (1);

-- this is a long comment about something that needs a lot of words
-- this is also a long comment that needs a lot of words
select SETVAL('foo_id_seq', 100000);

Run the following command using pg_format version 5.5:

pg_format --wrap-comment --wrap-limit 88 foo.sql

This should give you:

INSERT INTO foo (id)
    VALUES (1);

-- this is a long comment about something that needs a lot of words
--  this is also a long comment that needs a lot of words
SELECT
    SETVAL('foo_id_seq', 100000);

Run the command again, which should give you:

INSERT INTO foo (id)
    VALUES (1);

-- this is a long comment about something that needs a lot of words
--   this is also a long comment that needs a lot of words
SELECT
    SETVAL('foo_id_seq', 100000);

Notice the extra space at the start of the second comment line. This is an issue as comparing the outputs from the old file to the formatted one are always different, which means CI can't check if the files with long comments are correctly formatted.

This seems to be related to the perceived line length of the formatter as this problem goes away if I decrease the length of the second comment. For example, changing the second comment to --this is a short comment yield idempotent results.

I'm happy to provide further details if requested.

darold commented 1 year ago

Correct, commit 3815a2a might fix this issue.