-
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
Add configurable field-specific validator overrides to set filter operators as optional #1025
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1025 +/- ##
==========================================
+ Coverage 92.33% 92.34% +0.01%
==========================================
Files 67 67
Lines 3784 3790 +6
==========================================
+ Hits 3494 3500 +6
Misses 290 290
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
b495177
to
07bff38
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.
Seems good to me.
Would it be possible to add a test?
Just a very simple one for checking sub-strings in a valid and invalid case, maybe?
Simply making sure the feature works.
Also - I was thinking - the models support levels are taken from what we set in the models, correct? Does this mean we need to update these as well - or is it merely because we're hitting a sub-criterium here that the support level needs to be overridden like this?
Bit tricky to test that the tests are treated as optional (we don't test this anywhere else) since it depends on the server. The validator "config" is very much not user code, and the config values are basically hardcoded to reflect the specification. We could add a validator for the config field, but I think it would be overly restrictive in the dev edge case that someone wants to patch the validator for their own system.
Yes, this doesn't affect the field |
Fair enough. Would it make sense with a unit test at least for the functions? Making sure the |
We could really go to town on unit tests of edge cases with the validator (mocking servers/requests, checking particular requests etc. are made) but I don't think it is worth the effort at this stage (I don't have the bandwidth, at least). I've tested this on the OQMD (who the issue affects) and a few other providers. We should see any bugs popping up in the dashboard PR runs (when we next release) which I think provides better coverage than we could in this code. |
Closes #1024.