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

fix: add unique constraint #250

Merged
merged 1 commit into from
Apr 18, 2024
Merged

fix: add unique constraint #250

merged 1 commit into from
Apr 18, 2024

Conversation

guilhas07
Copy link
Contributor

@guilhas07 guilhas07 commented Apr 17, 2024

This is an attempt to solve #249 .

I updated a test to include the UNIQUE keyword. However, I'm having trouble running the tests (even before making these changes) to verify if everything is correct:

$ make test
test
make: *** [Makefile:26: test] Error 1

EDIT: found how to run the tests. There is one that is failing (but already was failing before I believe).
Maybe the tree-sitter-cli requirement should be mentioned in CONTRIBUTING.md?

Please let me know if there is something else that I should do/change!

@guilhas07 guilhas07 changed the title fix: add unique constraint (closes #249) fix: add unique constraint Apr 17, 2024
Copy link
Collaborator

@matthias-Q matthias-Q left a comment

Choose a reason for hiding this comment

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

LGTM, just adopt the tests that we cover both variants

@@ -30,7 +30,7 @@ Create table multiple columns

CREATE TABLE my_table (
id BIGINT NOT NULL PRIMARY KEY,
date DATE DEFAULT NULL ASC
date DATE DEFAULT NULL UNIQUE ASC
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you keep the old line and add a new line like

  date DATE DEFAULT NULL ASC,
  date2 DATE DEFAULT NULL UNIQUE ASC

Copy link
Collaborator

Choose a reason for hiding this comment

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

tree-sitter changed some thing with regards on query matching order in one of the last releases. Thats why tests are failing for you. We are aware of that and planning to update tree-sitter. The requirement for tree-sitter-cli is pinned in package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you keep the old line and add a new line like

Done!

The requirement for tree-sitter-cli is pinned in package.json

Ohh ok, so there is actually no need to run make test and instead we should run npm test?

Strange that both differ:

  • make test:
    ✓ union.sql (13 assertions)
    ✗ query.sql
      Failure - row: 0, column: 7, expected highlight 'function.call', actual highlights: 'type'
make: *** [Makefile:26: test] Error 1
  • npm test:
  ✓ union.sql (13 assertions)
  ✓ query.sql (14 assertions)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can still use make test but you need tree-sitter-cli 0.20.*

The "fix" for this issue is to change the order of tree-sitter queries highlights.scm. But that is not related to your MR.

@matthias-Q matthias-Q merged commit ea878da into DerekStride:main Apr 18, 2024
4 checks passed
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.

2 participants