evidence-dev / evidence

Business intelligence as code: build fast, interactive data visualizations in pure SQL and markdown
https://evidence.dev
MIT License
3.44k stars 167 forks source link

fix: remove comments and trailing `;` in sql query #1887

Open AyushAgrawal-A2 opened 1 month ago

AyushAgrawal-A2 commented 1 month ago

Description

closes #733 closes #1735 closes #1450

Changes:

Before: image

After: image

Checklist

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 8300b3b5752f20d911609d0be2eb60763cf548df

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages | Name | Type | | --------------------------------- | ----- | | @evidence-dev/preprocess | Patch | | @evidence-dev/sdk | Patch | | @evidence-dev/evidence | Patch | | @evidence-dev/components | Patch | | @evidence-dev/component-utilities | Patch | | @evidence-dev/core-components | Patch | | my-evidence-project | Patch | | evidence-test-environment | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify[bot] commented 1 month ago

Deploy Preview for evidence-development-workspace ready!

Name Link
Latest commit 8300b3b5752f20d911609d0be2eb60763cf548df
Latest deploy log https://app.netlify.com/sites/evidence-development-workspace/deploys/66468d37918b200008c4771c
Deploy Preview https://deploy-preview-1887--evidence-development-workspace.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 month ago

Deploy Preview for next-docs-evidence ready!

Name Link
Latest commit 8300b3b5752f20d911609d0be2eb60763cf548df
Latest deploy log https://app.netlify.com/sites/next-docs-evidence/deploys/66468d37d8a5b8000874edd8
Deploy Preview https://deploy-preview-1887--next-docs-evidence.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

csjh commented 1 month ago

Thanks for the PR!

One concern: seems like queries with string-enclosed "comments" are also caught here, which could be an issue if it's used as a delimiter or such in a string input (i.e. New York -- NY)

AyushAgrawal-A2 commented 1 month ago

@csjh Sorry, I missed that edge case. Tackling this requires parsing the query, I don't think it can be done via simple regex replace. I have added a helper function to do so.

This is required in 3 packages. In order to avoid cross package dependencies I have added this in all three packages. But I don't believe duplicate code is good. Please suggest if there is a common place to place this function. I tried placing it in db-commons but it create a cyclic dependency betweet QueryStore and db-commons.

Before: image

After: image

ItsMeBrianD commented 3 days ago

This is required in 3 packages. In order to avoid cross package dependencies I have added this in all three packages. But I don't believe duplicate code is good. Please suggest if there is a common place to place this function. I tried placing it in db-commons but it create a cyclic dependency betweet QueryStore and db-commons.

This should only be used in the query-store (which is now sdk/usql/query.js), and that should cover the impacted area

netlify[bot] commented 2 days ago

Deploy Preview for evidence-test-env ready!

Name Link
Latest commit 8300b3b5752f20d911609d0be2eb60763cf548df
Latest deploy log https://app.netlify.com/sites/evidence-test-env/deploys/66468d371e8325000885772f
Deploy Preview https://deploy-preview-1887--evidence-test-env.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

AyushAgrawal-A2 commented 2 days ago

This is required in 3 packages. In order to avoid cross package dependencies I have added this in all three packages. But I don't believe duplicate code is good. Please suggest if there is a common place to place this function. I tried placing it in db-commons but it create a cyclic dependency betweet QueryStore and db-commons.

This should only be used in the query-store (which is now sdk/usql/query.js), and that should cover the impacted area

@ItsMeBrianD Done image

ItsMeBrianD commented 2 days ago

This is ready to be merged, but going to hold off until after the release so there is a testing window on next, thanks for the contribution!