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

Add more PostgreSQL keywords and phrases #821

Merged

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Jan 5, 2025

Closes #809

- user_job_posting AS materialized (
+ user_job_posting AS MATERIALIZED (
- ON CONFLICT DO nothing
+ ON CONFLICT DO NOTHING
- ON CONFLICT (foreign_cloudinary_asset_id) DO nothing
+ ON CONFLICT (foreign_cloudinary_asset_id) DO NOTHING
- WITH ordinality nids
+ WITH ORDINALITY nids
- ORDER BY job_posting_approved_at DESC nulls last,
+ ORDER BY job_posting_approved_at DESC NULLS LAST,
+ DROP MATERIALIZED VIEW if EXISTS discover_independents_user_account_v3 CASCADE;
+ DROP MATERIALIZED VIEW IF EXISTS discover_independents_user_account_v3 CASCADE;
- RESET role;
+ RESET ROLE;

Some specific examples:

-- Before
SELECT
  fruit,
  pos 
FROM
  unnest(
    ARRAY['apple', 'banana', 'cherry']
  )
WITH
  ordinality AS fruits (fruit, pos);

-- After
SELECT
  fruit,
  pos 
FROM
  unnest(
    ARRAY['apple', 'banana', 'cherry']
  )
WITH ORDINALITY AS fruits (fruit, pos);

Copy link

codesandbox-ci bot commented Jan 5, 2025

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 karlhorky changed the title Add more keywords and phrases Add more PostgreSQL keywords and phrases Jan 5, 2025
@nene
Copy link
Collaborator

nene commented Jan 5, 2025

Thanks for the patch. Replying also to your comment in #820.

I agree that this using identifierCase: lower and keywordCase: upper is something that most people would want. It's just that it's not possible to fully achieve the correct behavior with the architecture of SQL Formatter. Some of these issues can be patched, like you've done in this PR, and I'm happy to accept such patches, but that can only take us so far. In particular it's mostly safe to introduce more multi-keyword syntax to be detected. However, when it comes to single keywords, we're getting into a problematic territory. Like in here, you've added TEMPLATE to keywords list, but this is not a reserved keyword - it could equally well be used as a name of a table or column, in which case it will now end up being uppercased.

SELECT template FROM tbl;

CREATE DATABASE foo TEMPLATE my_template;

Unfortunately SQL Formatter doesn't understand the context to know when TEMPLATE is used as a keyword and when it's used as an identifier.

@karlhorky
Copy link
Contributor Author

Right, I was considering whether this 'TEMPLATE' change would be rejected for that reason.

What I would really want is such a syntax (inspired by regular expressions), to allow for multi-keyword syntax with non-keyword parts in between:

"CREATE DATABASE '[^']+' TEMPLATE"

But I'm guessing that would be a larger change.

For now, I could remove the 'TEMPLATE' keyword instead, if that's a desirable path forward.

@nene
Copy link
Collaborator

nene commented Jan 5, 2025

Yeah, that would be a major undertaking. There are several problems:

  • These items between keywords can have quite a bit of syntax by themselves. Like the name of database could be either a quoted or unquoted identifier.
  • The actual syntax supported by the SQL dialect might be more complex than what one might guess from a bug report. See PostgreSQL docs for CREATE DATABASE.
  • There would also need to be a mechanism to distinguish which part of this expression are the keywords (that should e.g. be uppercased) and which parts are not.

For now, yeah, I see no better approach than just dropping TEMPLATE from keywords list.

@nene nene merged commit 366bd7b into sql-formatter-org:master Jan 5, 2025
2 checks passed
@karlhorky
Copy link
Contributor Author

@nene makes sense, thanks for the consideration and guidance :)

I'm guessing this and #820 will be included in a new published release in either the next patch or minor:

@nene
Copy link
Collaborator

nene commented Jan 5, 2025

I released these in 15.4.9

@karlhorky karlhorky deleted the add-more-clauses-and-phrases branch January 5, 2025 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FORMATTING] a few cases of unexpected identifierCase "lower" behavior
2 participants