Skip to content

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

Merged
surister merged 4 commits into
crate:mainfrom
surister:js_error_collector
Jul 8, 2024
Merged

Javascript: Implement ExceptionCollectorListener and make it default behaviour.#38
surister merged 4 commits into
crate:mainfrom
surister:js_error_collector

Conversation

@surister

@surister surister commented Jun 17, 2024

Copy link
Copy Markdown
Contributor

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 force-pushed the js_error_collector branch from 40d75ab to d09333f Compare July 6, 2024 14:50
@surister surister requested review from alexdametto and amotl July 6, 2024 16:15
@surister surister marked this pull request as ready for review July 6, 2024 16:16
@surister

surister commented Jul 6, 2024

Copy link
Copy Markdown
Contributor Author

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

@amotl amotl left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread cratedb_sqlparse_js/README.md Outdated
Comment thread cratedb_sqlparse_js/README.md Outdated
Comment thread cratedb_sqlparse_js/README.md Outdated
Comment thread cratedb_sqlparse_js/cratedb_sqlparse/AstBuilder.js Outdated
Comment thread cratedb_sqlparse_js/README.md Outdated
Comment thread cratedb_sqlparse_js/README.md Outdated
Comment thread cratedb_sqlparse_js/README.md Outdated
Comment thread cratedb_sqlparse_js/README.md
@surister surister force-pushed the js_error_collector branch from 2457c17 to d148041 Compare July 7, 2024 11:08
@surister

surister commented Jul 7, 2024

Copy link
Copy Markdown
Contributor Author

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.

@alexdametto alexdametto left a comment

Copy link
Copy Markdown
Contributor

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

surister commented Jul 8, 2024

Copy link
Copy Markdown
Contributor Author

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
@matriv

matriv commented Jul 8, 2024

Copy link
Copy Markdown

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

@surister surister deleted the js_error_collector branch April 8, 2025 09:41
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