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

Add new filtering operators #123

Merged
merged 7 commits into from
Oct 19, 2018
Merged

Add new filtering operators #123

merged 7 commits into from
Oct 19, 2018

Conversation

burov
Copy link

@burov burov commented Oct 18, 2018

I add new filtering operators, such as in(val in array for number and string arrays) and := (Insensitive equal operator)

Copy link
Contributor

@Evgeniy-L Evgeniy-L left a comment

Choose a reason for hiding this comment

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

Overall looks good. I see that updates for documentation is missing. Please update readme accordingly with new operators

@@ -2,11 +2,11 @@ package gorm

import (
"context"
"github.com/infobloxopen/atlas-app-toolkit/query"
Copy link
Contributor

Choose a reason for hiding this comment

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

why the order of imports was changed. Typically we try to break imports in 3 different sections with a newline between each one to separate standard imports, third party imports, imports from infobloxopen

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, i think it because i did use standart go format tool and it perform sorting imports as one of format operation, i will try be more careful about this

README.md Outdated
@@ -1004,6 +1004,8 @@ Literal values include numbers (integer and floating-point), and quoted (both si
| or | Logical OR | price <= 3.5 or price > 200 |
| not | Logical NOT | not price <= 3.5 |
| () | Grouping | (priority == 1 or city == ‘Santa Clara’) and price > 100 |
| := | Insensitive equal | city := 'SaNtA ClArA' |

Choose a reason for hiding this comment

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

Why we don't add a literal representation of the operator?(ie)
Seems like any other operator has it.

Copy link
Author

@burov burov Oct 19, 2018

Choose a reason for hiding this comment

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

Evgeniy-L sent me documentation and i did all folow the documentation, in this document string representation not exist for ':=' operator and i did't add it, if you think we need alias 'ie' for ':=' operator, i can add it

Copy link
Contributor

Choose a reason for hiding this comment

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

@dkukharau, @burov, ie doesn't look intuitively clear. Have you seen something like this anywhere it is is just something we just invented?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the idea of adding uncommon things

Choose a reason for hiding this comment

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

Ok, let's name it ieq, there is such operator in PowerShell.

Copy link
Author

@burov burov Oct 19, 2018

Choose a reason for hiding this comment

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

@Evgeniy-L what you think about this operator, what we need to do? Change alias to 'ieq' or remove alias ?

Copy link
Contributor

@Evgeniy-L Evgeniy-L Oct 19, 2018

Choose a reason for hiding this comment

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

ok, lets have it added as ieq

[]interface{}{"sOmeId", "egegeg", "Hello"},
nil,
nil,
},

Choose a reason for hiding this comment

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

Need a test case for ArrayNumberCondition as well - not a complex expression, just a simple one.

values := make([]string, 0)
for lexer.curChar != term || lexer.eof {
switch lexer.curChar {
case ' ', ',':
Copy link

@dkukharau dkukharau Oct 19, 2018

Choose a reason for hiding this comment

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

unicode.IsSpace should be used here to check a whitespace.

@@ -37,6 +36,9 @@ func TestFilteringLexer(t *testing.T) {
LeToken{},
LeToken{},
NullToken{},
InsensitiveEqToken{},

Choose a reason for hiding this comment

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

InToken is missing..

Choose a reason for hiding this comment

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

Also good to have negative tests for empty array([]), array with both strings and numbers(["abc", 1, 2]), unclosed array([1,2,)

Copy link

@dkukharau dkukharau left a comment

Choose a reason for hiding this comment

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

Overally good work! Go with merge after fixing minor notes.

Copy link
Contributor

@Evgeniy-L Evgeniy-L left a comment

Choose a reason for hiding this comment

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

Looks good. Please update related docs as well.

@burov burov merged commit 8a18827 into infobloxopen:master Oct 19, 2018
@burov burov deleted the new_collections_operators branch October 19, 2018 11:33
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.

3 participants