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

Fix filter so logical operations are constructed correctly #123

Merged
merged 2 commits into from
May 21, 2015

Conversation

jazdw
Copy link
Contributor

@jazdw jazdw commented May 14, 2015

See the following gist for what this fixes. Also allows for nicer filter.or({key: value}) type filtering.

https://gist.github.com/jazdw/2410f64b99fbd9c104fa

@jazdw
Copy link
Contributor Author

jazdw commented May 14, 2015

Note that #121 is related, perhaps sort and limit should be added to Filter as another operator type (so you would create them as filter.limit(10,20) for example) and then the whole query log could just be joined using filterCreator('and').apply(Filter.prototype, queryLog)

@kriszyp
Copy link
Member

kriszyp commented May 14, 2015

This looks like an excellent improvement to filtering. However, it looks like a unit testing is failing. Would you mind taking a look at the failing unit test?

@kriszyp
Copy link
Member

kriszyp commented May 14, 2015

Or is this dependent on #121 to address the unit test failure?

@jazdw
Copy link
Contributor Author

jazdw commented May 18, 2015

Hey Kris, the or method will work slightly differently now. I've fixed the unit test. I think it reads more naturally?
condition1.or(condition2) => OR(condition1, condition2)
condition1.or(condition2, condition3) => OR(condition1, condition2, condition3)
instead of
condition1.or(condition2) => AND(condition1, OR(condition2))
condition1.or(condition2, condition3) => AND(condition1, OR(condition2, condition3))

@kriszyp kriszyp merged commit fc8815a into dojo:master May 21, 2015
@kriszyp
Copy link
Member

kriszyp commented May 21, 2015

Looks good, thank you!

@jazdw jazdw deleted the fix-filter branch May 25, 2015 06:10
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.

2 participants