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

Improve dev workflow and add SQL code formatter #72

Closed
wants to merge 15 commits into from
Closed

Conversation

dqii
Copy link
Contributor

@dqii dqii commented Aug 21, 2023

No description provided.

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #72 (ff05df8) into main (b7e0e58) will decrease coverage by 0.09%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

❗ Current head ff05df8 differs from pull request most recent head f0a7695. Consider uploading reports for the commit f0a7695 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
- Coverage   83.10%   83.02%   -0.09%     
==========================================
  Files          11       11              
  Lines         876      854      -22     
  Branches      165      161       -4     
==========================================
- Hits          728      709      -19     
+ Misses         67       66       -1     
+ Partials       81       79       -2     

see 2 files with indirect coverage changes

@dqii dqii changed the title Test and development workflow iterations Improve dev workflow and add SQL code formatter Aug 22, 2023
@dqii dqii linked an issue Aug 22, 2023 that may be closed by this pull request
{
"recommendations": [
"twxs.cmake",
"bradymholt.pgformatter"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not commit commit custom editor configs in here. We will all have custom configs and we can each maintain those.
for pgformatter, we should have a make target also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for pgformatter, we should have a make target also.

I added a make target! It's called format-sql. Should I mention it in future docs?

The flow I plan to follow is that CI/CD will check if relevant files are mis-formatted, and throw an error if so. The user will need to format these files, whether through running make format-sql, VSCode auto-formatter, etc.

.vscode/settings.json Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CREATE FUNCTION ldb_generic_dist (real[], real[])
RETURNS real
AS 'MODULE_PATHNAME'
LANGUAGE C
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have the option of turning off pgFormatter formatting for parts of the file (e.g. via a magic-comment.
e.g. in clang-format, any code I write within the magic comments below will not be formatted

// clang-format off
[CODE HERE]
// clang-format on

We should use a formatter that has such an option and should use such an option for sql statements that are very similar. This allows to more easily tell what is different between similar SQL statements.

For example, in the above, the two ldb_generic_dist, and l2sq_dist only differ in their argument types and that is easier to quickly verify visually when we have long "unformatted" sql lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 -p | --placeholder RE : set regex to find code that must not be changed.

We could use this to achieve that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great!
Let's do that for these kind of repeating statements.

AS 'MODULE_PATHNAME' LANGUAGE C;
CREATE FUNCTION hnsw_handler (internal)
RETURNS index_am_handler
AS 'MODULE_PATHNAME'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a weak preference of not touching old update files. These are supposed to be immutable after the corresponding release. Formatting does not change the logic so not a big deal

@dqii
Copy link
Contributor Author

dqii commented Aug 22, 2023

Update

pgFormatter doesn't support magic comment ignores well. (It keeps adding a space to the next line, for example). In addition, it doesn't support CTEs well either (multiple Github issues about this).

I tried out pglast, which is based on libpg_query, which uses Postgres's parser. This handles all the Postgres SQL and pl/pgSQL corner cases nicely, but unfortunately doesn't handle the psql metacommands (e.g., \set \eqcho \copy)

@dqii dqii closed this Aug 24, 2023
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.

Add SQL code formatter
2 participants