-
Notifications
You must be signed in to change notification settings - Fork 42
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
Validate all non-optional :filter: examples from the spec #213
Conversation
b4808a5
to
4c30c05
Compare
26b1ef6
to
32d3501
Compare
6d4d10a
to
cd69a9e
Compare
747117b
to
8e7078d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casually looking through this to see what you've been up to - looking good! :)
Found some missing spaces in the query tests, you should just be able to apply the suggestions directly to fix them.
Also a question about making ResourceMapper
a meta-class.
Cheers! There's something going on with the actual mongo tests, I think we may just be testing more queries against mongo than we were previously, so I'll have to set up a better test environment locally to fix that. Mongo has lots of annoying things like |
This should all work now, some final queries:
|
No. We are testing the But on the other hand, this is testing the |
Cool, agreed. This needed a couple of extra tweaks after the optimism in my last comment, but this now seems to be working fine. I've summarised the changes in more detail above, but the missing piece over the weekend was adding an int class to the grammar to catch e.g. |
Codecov Report
@@ Coverage Diff @@
## master #213 +/- ##
==========================================
+ Coverage 87.15% 87.55% +0.39%
==========================================
Files 42 43 +1
Lines 1892 1912 +20
==========================================
+ Hits 1649 1674 +25
+ Misses 243 238 -5
Continue to review full report at Codecov.
|
b83c45c
to
ac85717
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An impressive PR and finally some more love to the validator 👍
I've left some comments, thoughts, and suggested changes - all more or less minor things.
@@ -92,6 +92,9 @@ def expression_phrase(self, arg): | |||
# without NOT | |||
return arg[0] | |||
|
|||
if list(arg[1].keys()) == ["$or"]: | |||
return {"$nor": arg[1]["$or"]} | |||
|
|||
# with NOT | |||
# TODO: This implementation probably fails in the case of `"(" expression ")"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still true? I guess it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not sure. I don't think the example is valid for the grammar anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea either. Although the example was put in for this grammar by @fekad I believe?
f56a42c
to
e549df4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final thing(s), then it's good to go on my part 👍 Great work @ml-evs !
As a final change, I've just added the no cover pragma to the validator client, as this is tested outside of py.test. |
Sure - or we should manually run those tests using |
I'm fine with later... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good to me - thanks for this @ml-evs !
Please squash your commits into reasonable chunks, and I'll approve for you to merge 👍
7201edf
to
d9ca8f8
Compare
- This avoids a user setting up an alias that users a mongo keyword.
d9ca8f8
to
1fc320b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, many thanks @ml-evs ! Great work.
This PR builds on #205 by scraping the spec for
:filter:
tags and pushing them through the validator.Changes:
HAS LENGTH value
becomesHAS LENGTH 1
)ResourceMapper
to properly test for thisLENGTH
by updating grammar