search

crate / cratedb-sqlparse

Parsing utilities to validate and split SQL statements for CrateDB.
Apache License 2.0
4 stars 1 forks source link

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

Closed surister closed 3 days ago

surister commented 3 weeks ago

Summary of the changes / Why this is an improvement

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

Checklist

Also adds support for typescript types, solving #52.

surister commented 4 days ago

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

surister commented 3 days ago

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.

surister commented 3 days ago

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)

matriv commented 3 days ago

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