[add] sql-formatter params #6724

ko-ya346 · 2024-01-21T14:36:25Z

What type of PR is this?

Description

This pull request introduces significant improvements to the query editor's formatting functionality in Redash. Previously, the query editor's format feature was limited to default settings. With this update, users can now utilize custom formatting settings defined in a configuration file, allowing for greater flexibility and personalization.

Key Changes:

Upgraded sql-formatter and @types/sql-formatter

The versions of sql-formatter and its corresponding TypeScript type definitions have been updated. This upgrade ensures better performance and compatibility with the new formatting features.

Loading Custom Settings from Configuration File in queryFormat.js
Implemented new functionality in queryFormat.js to read formatting settings from an external configuration file. This allows users to define their preferred formatting options, such as indentation style, keyword casing, and other SQL formatting standards.

This enhancement aims to provide a more user-friendly and customizable experience in the query editor, catering to diverse coding styles and preferences.

How is this tested?

Unit tests (pytest, jest)
E2E Tests (Cypress)
Manually
N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

gabrieldutra · 2024-01-22T12:55:42Z

package.json

@@ -72,7 +72,7 @@
    "react-grid-layout": "^0.18.2",
    "react-resizable": "^1.10.1",
    "react-virtualized": "^9.21.2",
-    "sql-formatter": "git+https://github.com/getredash/sql-formatter.git",
+    "sql-formatter": "^15.0.2",


FYI the main reason we had a custom sql-formatter maintained in getredash/sql-formatter is due to the lack of support to the parameters syntax ({{}}). Previously it would just add line breaks, breaking the query text, with this version it seems to throw an error when a query has them

@gabrieldutra
Thanks for the review.
The commit below eliminates the error that occurs when formatting is executed when query parameters are included.
Specifically, we specified {{}} in the paramTypes argument.

dabd3ed

I used this link as a reference.

https://github.com/sql-formatter-org/sql-formatter/blob/HEAD/docs/paramTypes.md

AndrewChubatiuk · 2024-01-24T11:15:36Z

@gabrieldutra sqlformatter supports now advanced configuration, so cases, that are covered in a fork can be easily implemented using a latest library version

ko-ya346 · 2024-01-30T00:52:08Z

@AndrewChubatiuk
Thanks to your advice I was able to perform the formatting function without any errors in cases involving query parameters. Thank you very much.

guidopetri · 2024-01-30T03:42:27Z

@ko-ya346 thanks for the PR! It'd be pretty nice to remove a manually-maintained dependency.

I set the checks to run on this PR, let's see if they pass.

codecov · 2024-01-30T03:48:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.41%. Comparing base (af0773c) to head (9a6b50a).
Report is 33 commits behind head on master.

❗ Current head 9a6b50a differs from pull request most recent head 7471d9a. Consider uploading reports for the commit 7471d9a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6724      +/-   ##
==========================================
- Coverage   63.82%   63.41%   -0.41%     
==========================================
  Files         161      162       +1     
  Lines       13060    13169     +109     
  Branches     1803     1819      +16     
==========================================
+ Hits         8335     8351      +16     
- Misses       4425     4522      +97     
+ Partials      300      296       -4

see 21 files with indirect coverage changes

guidopetri · 2024-01-30T03:58:56Z

@ko-ya346 it looks like the frontend unit tests are failing. Would you mind investigating and seeing if they pass on your local machine?

AndrewChubatiuk · 2024-01-30T04:03:31Z

looks like this module requires changes in webpack config and its dependencies

ko-ya346 · 2024-01-31T00:19:35Z

@guidopetri @AndrewChubatiuk

I ran fromtend unit test on my local machine and it failed.
The stdout from the run is as follows.
I will investigate the cause after this, but if you know of a solution I would appreciate it if you could let me know.

$ make frontend-unit-tests
CYPRESS_INSTALL_BINARY=0 PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=1 yarn --frozen-lockfile
yarn install v1.22.19
$ cd viz-lib && yarn link --link-folder ../.yarn
yarn link v1.22.19
warning There's already a package called "@redash/viz" registered. This command has had no effect. If this command was run in another folder with the same name, the other folder is still linked. Please run yarn unlink in the other folder if you want to register this folder.
Done in 0.03s.
[1/5] Validating package.json...
[2/5] Resolving packages...
success Already up-to-date.
$ (cd viz-lib && yarn --frozen-lockfile && yarn build:babel) && yarn link --link-folder ./.yarn @redash/viz
yarn install v1.22.19
[1/4] Resolving packages...
success Already up-to-date.
Done in 0.35s.
yarn run v1.22.19
$ yarn type-gen && yarn build:babel:base
$ tsc --emitDeclarationOnly
$ babel src --out-dir lib --source-maps --ignore 'src/**/*.test.js' --copy-files --no-copy-ignored --extensions .ts,.tsx,.js,.jsx
Browserslist: caniuse-lite is outdated. Please run:
  npx update-browserslist-db@latest
  Why you should do it regularly: https://github.com/browserslist/update-db#readme
Successfully compiled 169 files with Babel (2770ms).
Done in 6.99s.
yarn link v1.22.19
success Using linked package for "@redash/viz".
Done in 0.04s.
Done in 8.28s.
yarn test
yarn run v1.22.19
$ run-s type-check jest
$ tsc --noEmit --project client/tsconfig.json
client/app/lib/queryFormat.ts:2:24 - error TS2307: Cannot find module 'sql-formatter' or its corresponding type declarations.

2 import { format } from "sql-formatter";
                         ~~~~~~~~~~~~~~~


Found 1 error.

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "type-check" exited with 2.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
make: *** [Makefile:45: frontend-unit-tests] エラー 1

gabrieldutra · 2024-01-31T13:37:32Z

package.json

@@ -96,7 +96,7 @@
    "@types/prop-types": "^15.7.3",
    "@types/react": "^16.14.2",
    "@types/react-dom": "^16.9.10",
-    "@types/sql-formatter": "^2.3.0",
+    "@types/sql-formatter": "^4.0.1",


From https://www.npmjs.com/package/@types/sql-formatter, it seems sql-formatter now provides its own types? Probably can remove this package, which may be what's failing CI

@gabrieldutra
Thanks for the review.
You are correct, removing @types/sql-formatter did not affect the behavior.

ko-ya346 · 2024-02-03T15:16:27Z

The frontend tests now pass on my local machine following the recent commit.

guidopetri

@ko-ya346 thanks for this change! I'm excited to give this a try :)

ko-ya346 · 2024-02-06T15:52:05Z

@guidopetri
I may have made an incorrect commit, possibly disregarding your review.

ko-ya346 · 2024-02-08T01:17:31Z

The frontend-e2e-tests are failing, although they passed previously. I cannot determine the cause from the logs, so I will attempt to reproduce the issue for further investigation.

guidopetri · 2024-02-08T01:21:05Z

I'm not sure that the tests failing is related to your changes. Let me investigate first before you spend too much time on a red herring. Do the tests pass for you on your local?

ko-ya346 · 2024-02-08T02:33:05Z

@guidopetri
Thank you for your feedback.
I ran the commands locally and confirmed that everything passed.
It seems that the issue is not related to my code changes.

yarn cypress build
yarn cypress start
yarn cypress run

guidopetri · 2024-02-08T02:47:18Z

Thanks @ko-ya346 :) I won't merge this in until our CI on master is passing again, but it does seem like this PR is done. Thank you so much for your work, and hopefully soon we'll merge this in! I'll ping you again if anything else is necessary, but I appreciate all your work.

AndrewChubatiuk · 2024-04-25T10:35:47Z

Hey @ko-ya346
Could you please rebase your branch?

…ameters

AndrewChubatiuk · 2024-04-25T15:46:52Z

Tests have passed!
LGTM

AndrewChubatiuk · 2024-04-26T09:52:45Z

@justinclift could you please merge it?

ko-ya346 · 2024-05-07T11:48:03Z

@guidopetri
test passed!
could you please merge it?

gabrieldutra reviewed Jan 22, 2024

View reviewed changes

gabrieldutra reviewed Jan 31, 2024

View reviewed changes

guidopetri previously approved these changes Feb 6, 2024

View reviewed changes

guidopetri enabled auto-merge (squash) February 6, 2024 01:53

auto-merge was automatically disabled February 6, 2024 15:45
Head branch was pushed to by a user without write access

ko-ya346 dismissed guidopetri’s stale review via 2f64bb3 February 6, 2024 15:45

ko-ya346 and others added 10 commits April 26, 2024 00:16

[change] sql-formatter version

bfa9c26

[add] sql-formatter config

daf00ab

[update] sql-formatter

bd0eef0

refuctoring

55ce747

refuctoring

8daccf7

[fix] set paramsType for the format of the query, including query par…

4b7e2ce

…ameters

Restyled by prettier

fbac96b

[del] @types/sql-formatter

9515714

upgrade typescript

0fa7c46

[add] .\s- -> paramTypes:custom

3e11242

ko-ya346 added 3 commits April 26, 2024 00:16

[change] typescript

55ce8eb

[change] formatterConfig .js -> .json

9d128c4

[add] referrence sql-formatter

7471d9a

ko-ya346 force-pushed the add/sql-formatter-params branch from 9a6b50a to 7471d9a Compare April 25, 2024 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[add] sql-formatter params #6724

[add] sql-formatter params #6724

ko-ya346 commented Jan 21, 2024 •

edited

gabrieldutra Jan 22, 2024

ko-ya346 Jan 30, 2024 •

edited

AndrewChubatiuk commented Jan 24, 2024

ko-ya346 commented Jan 30, 2024

guidopetri commented Jan 30, 2024

codecov bot commented Jan 30, 2024 •

edited

guidopetri commented Jan 30, 2024

AndrewChubatiuk commented Jan 30, 2024 •

edited

ko-ya346 commented Jan 31, 2024

gabrieldutra Jan 31, 2024

ko-ya346 Feb 1, 2024

ko-ya346 commented Feb 3, 2024

guidopetri left a comment

ko-ya346 commented Feb 6, 2024

ko-ya346 commented Feb 8, 2024

guidopetri commented Feb 8, 2024

ko-ya346 commented Feb 8, 2024

guidopetri commented Feb 8, 2024

AndrewChubatiuk commented Apr 25, 2024

AndrewChubatiuk commented Apr 25, 2024

AndrewChubatiuk commented Apr 26, 2024

ko-ya346 commented May 7, 2024

[add] sql-formatter params #6724

Are you sure you want to change the base?

[add] sql-formatter params #6724

Conversation

ko-ya346 commented Jan 21, 2024 • edited

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

gabrieldutra Jan 22, 2024

Choose a reason for hiding this comment

ko-ya346 Jan 30, 2024 • edited

Choose a reason for hiding this comment

AndrewChubatiuk commented Jan 24, 2024

ko-ya346 commented Jan 30, 2024

guidopetri commented Jan 30, 2024

codecov bot commented Jan 30, 2024 • edited

Codecov Report

guidopetri commented Jan 30, 2024

AndrewChubatiuk commented Jan 30, 2024 • edited

ko-ya346 commented Jan 31, 2024

gabrieldutra Jan 31, 2024

Choose a reason for hiding this comment

ko-ya346 Feb 1, 2024

Choose a reason for hiding this comment

ko-ya346 commented Feb 3, 2024

guidopetri left a comment

Choose a reason for hiding this comment

ko-ya346 commented Feb 6, 2024

ko-ya346 commented Feb 8, 2024

guidopetri commented Feb 8, 2024

ko-ya346 commented Feb 8, 2024

guidopetri commented Feb 8, 2024

AndrewChubatiuk commented Apr 25, 2024

AndrewChubatiuk commented Apr 25, 2024

AndrewChubatiuk commented Apr 26, 2024

ko-ya346 commented May 7, 2024

ko-ya346 commented Jan 21, 2024 •

edited

ko-ya346 Jan 30, 2024 •

edited

codecov bot commented Jan 30, 2024 •

edited

AndrewChubatiuk commented Jan 30, 2024 •

edited