CESQL v1 fixes #1066
CESQL v1 fixes #1066
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
…correct type Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
…rror types Signed-off-by: Calum Murray <cmurray@redhat.com>
…or types Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
There is one more change I would like to include (either as a follow up or in this PR), but I wasn't sure what the API should be for it: Currently this SDK only supports the "fail fast" error handling mode, where the CESQL engine returns as soon as it encounters an error. However, I would like to allow users to choose which error handling strategy they would like to use (and then maybe default to fail fast to avoid breaking changes as much as possible). Any ideas on what a good API could be for that? |
|
rebase is needed |
|
@duglin can you re-check? |
|
@duglin for second approval/merge |
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
LGTM, thanks for the work, @Cali0707 |
|
rebase needed |
sql/v2/errors/errors.go
Outdated
|
|
||
| return cesqlError{ | ||
| kind: parseError, | ||
| message: strings.Join(errorMessages, ","), |
There was a problem hiding this comment.
Bold assumption that "," will never be used in an error message.... ever. I would have thought that "\n" would be better (or a less likely character, like "|", if you still wanted a single line) so that people could more easily split this into individual errors later on if needed. Not a show-stopper though.
sql/v2/errors/errors.go
Outdated
|
|
||
| func IsGenericError(err error) bool { | ||
| if cesqlErr, ok := err.(cesqlError); ok { | ||
| return cesqlErr.kind < parseError || cesqlErr.kind > functionEvaluationError |
There was a problem hiding this comment.
This feels risky to me. If someone adds a new error type will they remember to update this "return" line? I wonder if it would be safer to add a dummy error type to the end of the "const" list and then say >= dummyLastError here instead?
There was a problem hiding this comment.
I'd probably also use "0" instead of "parseError" in case someone adds a new item to the front of the const list.
| @@ -56,6 +64,10 @@ tests: | |||
| - name: abc is not equal to abc (diamond operator) | |||
| expression: "'abc' <> 'abc'" | |||
| result: false | |||
| - name: Diamond operator returns false when encountering a missing attribute | |||
There was a problem hiding this comment.
Funny, never heard it called "Diamond operator" before. I like it! Now I feel like I've been missing out all of these years.
| return false | ||
| case AnyType: | ||
| // by default, return false | ||
| return false |
There was a problem hiding this comment.
I don't have a strong opinion on this one, but I'm curious why it defaults to "false" (here and before) instead of "nil" ?
There was a problem hiding this comment.
Or is it just because it needs to be of a well-known type, and "nil" doesn't fit that requirement?
There was a problem hiding this comment.
Throughout the spec whenever the return type is ambiguous we always have it defaulting to Boolean, which has a zero value of false. So, this way that works without much effort on our part in the SDK
|
Just a few minor comments, none should stop this from being merged though. |
Signed-off-by: Calum Murray <cmurray@redhat.com>
- Parse errors are joined by "|" instead of "," - Safer checking of generic errors Signed-off-by: Calum Murray <cmurray@redhat.com>
This PR updates the CESQL implementation here to match the changes made in the spec v1 that was approved today. Since there are a lot of changes, I've tried to organize them by commit