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 in and nin operators #10

Closed
wants to merge 1 commit into from
Closed

add in and nin operators #10

wants to merge 1 commit into from

Conversation

ashtonian
Copy link
Contributor

resolves #8

Test currently fails due to deep value check of slices failing. Not sure what the goal is of the check so I'm hesitant to fix it.

@ashtonian
Copy link
Contributor Author

@a8m any thoughts on this or assistance on how i should fix the tests?

@a8m
Copy link
Owner

a8m commented Mar 12, 2019

Hey @ashtonian!
Thanks for your contribution, and sorry for the delayed response. I just returned back from a vacation.

any thoughts on this or assistance on how i should fix the tests?

I would be happy to clone this branch and check the test failures. Also, I would like to see integration tests as well (see the rql/integration directory).

I have a different suggestion for how the ValidateFn/ConvertFn will deal with slice of params, and I would like to know what do you think about it.

func (p *parseState) field(f *field, v interface{}) {
  // ...
  fn := f.ValidateFn
  if op == NIN || op == IN {
    fn = validateSlice(fn)
  }
  must(fn(opVal), "invalid datatype for field %q", f.Name)
  // code continues the same
  // ...
}

func validateSlice(fn ValidateFn) ValidateFn {
  return func(v interface{}) error {
     vs, ok := v.([]interface{})
     // handle error.
     for _, v := range vs {
       if err := fn(v); err != nil {
         return err
       }   
     }
   }
}

Just to share - I didn't know that the $nin operator exists in MongoDB, and at the start, I thought to implement the NOT IN operation with the "$not": { "$in": <val> } syntax.

This led me to discover that the $not operator is missing and it should be added.

@ashtonian
Copy link
Contributor Author

I was hoping there was a better place to tack it in but didn't see it, looks like that'd be much cleaner ha. I'll update shortly and add some integration tests. Thank you for the response!

@ashtonian ashtonian mentioned this pull request Jul 17, 2019
@ashtonian
Copy link
Contributor Author

closing in favor of new pr

@ashtonian ashtonian closed this Jul 17, 2019
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.

IN and NOTIN operators
2 participants