Skip to content

add comparator $contains #6

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

Merged
merged 1 commit into from
Dec 3, 2014
Merged

Conversation

ludmillaZ
Copy link
Contributor

need a contains for filter

@clue
Copy link
Owner

clue commented Oct 29, 2014

Thanks for your contribution, the code looks good to me and your addition is very much appreciated! 👍

However, the reason for not having this merged in yet is that this projects aims to be the reference implementation of the https://github.com/clue/json-query-language specification. The spec (draft) currently makes no mention of an $contains operator. I'd love to discuss this in the context of the specification, so a new issue over there would be very much appreciated.

I can certainly see where you're coming from and see some need for additional comparision operators. However, once we start adding a $contains operator, we'll likely see a need for similar operators like $startsWith and $endsWith as well. Also, the name $contains does not imply a data type, so is this limited to substring matching (perhaps we should consider a $regex operator?) or does this also apply to arrays?

@clue
Copy link
Owner

clue commented Nov 13, 2014

Also, the name $contains does not imply a data type […]

After some further consideration, this turns out to be a good thing. The future $contains operator should work on strings (substring matching), arrays (element search) and objects (key lookup).

The specification for the $contains operator is being worked on here: clue/json-query-language#9

Once then new spec has been released, we should update this PR.

@clue
Copy link
Owner

clue commented Nov 27, 2014

Just FYI: The spec has been updated and the new $contains operator has been merged. The spec has not been released yet.

@clue clue merged commit a6bfaea into clue:master Dec 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants