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

Javascript: Implement ExceptionCollectorListener and make it default behaviour. #38

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

surister
Copy link
Collaborator

@surister surister commented Jun 17, 2024

Summary of the changes / Why this is an improvement

Implements #30 #36 #37 #34 to the Javascript target
Solves #2

Checklist

  • Link to issue this PR refers to (if applicable):
  • CLA is signed

Also adds support for typescript types, solving #52.

@surister surister marked this pull request as ready for review July 6, 2024 16:16
@surister
Copy link
Collaborator Author

surister commented Jul 6, 2024

The PR might be to big to manually inspect/test, fyi it's 'already tested' (Since it's almost identical to the Python target and the test pass)

I'm interested on your opinion @alexdametto of the resulting API, you can quickly check it in https://github.com/crate/cratedb-sqlparse/pull/38/files#diff-111a35f00d7bdf2e15b6a32750f253243ae44df87bcf0465a2fbfd7178db2dc1

@amotl amotl requested a review from matriv July 7, 2024 10:23
Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Thanks a stack! I've just added a few nit suggestions about punctuation, wording, and formatting, to accept at your disposal.

cratedb_sqlparse_js/README.md Outdated Show resolved Hide resolved
cratedb_sqlparse_js/README.md Outdated Show resolved Hide resolved
cratedb_sqlparse_js/README.md Outdated Show resolved Hide resolved
cratedb_sqlparse_js/cratedb_sqlparse/AstBuilder.js Outdated Show resolved Hide resolved
cratedb_sqlparse_js/README.md Outdated Show resolved Hide resolved
cratedb_sqlparse_js/README.md Outdated Show resolved Hide resolved
cratedb_sqlparse_js/README.md Outdated Show resolved Hide resolved
@surister
Copy link
Collaborator Author

surister commented Jul 7, 2024

With the latest commit types generation is added, it works fine on my end.

installing the package locally in cratedb-alt-admin:

image

There are still some types, that are not being picked up, I do not know why, for example Metadata.withProperties. I set the jsdoc to object but any is generated. We can improve these on later iterations.

Copy link
Contributor

@alexdametto alexdametto left a comment

Choose a reason for hiding this comment

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

I'm interested on your opinion @alexdametto of the resulting API, you can quickly check it in https://github.com/crate/cratedb-sqlparse/pull/38/files#diff-111a35f00d7bdf2e15b6a32750f253243ae44df87bcf0465a2fbfd7178db2dc1

Amazing job. 💯 I really like it, especially the raise_exception and the exception details, these things are definitely needed in https://github.com/crate/cloud/issues/1888.

Could be possible to add also the line number of where the error occurs? I have seen that we have it inside stmt.exception.errorMessage but it would be good to have it stored in a separate field.

@surister
Copy link
Collaborator Author

surister commented Jul 8, 2024

I'm interested on your opinion @alexdametto of the resulting API, you can quickly check it in https://github.com/crate/cratedb-sqlparse/pull/38/files#diff-111a35f00d7bdf2e15b6a32750f253243ae44df87bcf0465a2fbfd7178db2dc1

Amazing job. 💯 I really like it, especially the raise_exception and the exception details, these things are definitely needed in crate/cloud#1888.

Could be possible to add also the line number of where the error occurs? I have seen that we have it inside stmt.exception.errorMessage but it would be good to have it stored in a separate field.

There is console.log(stmt.exception.line, stmt.exception.column)

@surister surister merged commit c9b0c84 into crate:main Jul 8, 2024
7 checks passed
@matriv
Copy link

matriv commented Jul 8, 2024

Sorry for the late review, looks good! thx a lot for the effort @surister !

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.

4 participants