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
Overhaul of filter transformers, mappers and response fields #797
Conversation
Codecov Report
@@ Coverage Diff @@
## master #797 +/- ##
==========================================
- Coverage 92.68% 92.68% -0.01%
==========================================
Files 68 68
Lines 3677 3759 +82
==========================================
+ Hits 3408 3484 +76
- Misses 269 275 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
6d13f3e
to
805d914
Compare
8158e2e
to
9ab9858
Compare
0a7d14a
to
b40d191
Compare
@CasperWA: this might be an over-interpretation of the spec. Looks like other implementations just return |
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 can only provide some more superficial comments. I think the biggest issues are
- the increased number of terms for the same thing (quantity, property, attribute, field). I know I contributed to that.
- I feel that the quantities related functionality belongs into the ResourceMapper classes.
f2e96f0
to
94bf368
Compare
…oses #743) - Add entry resource class attribute to mappers, use that to scrape attributes - Use new BaseTransformer to handle aliasing in MongoTransformer - Move definitions of ES quantities away from server and into mapper - Improve MongoFilterTransformer tests by using OPTIMADE queries directly - Add more ES FilterTransformer tests for KNOWN/UNKNOWN and fix weird dimension_types test - Extract as much info from schemas to construct mappers - Tweak and use retrieve_queryable_properties in mapper - Provide clearer error message for ES relationship filtering - Docstrings and annotations for transformers - Make reversed_operator_map and quantity_type private
…x does not match a known provider (closes #263) - Elasticsearch fix for provider fields - Add warning checks to tests - Fallback to no providers - Passthrough for 'other provider' fields
26d4a57
to
444b1b6
Compare
- Switch to single underscore for quantity methods, update docs for Quantity/ElasticsearchQuantity - Add docstrings for BaseTransformer attributes - Apply suggestions from code review - Docstring tweaks and less verbose mkdocs in CI - Remove seemingly defunct REQUIRED_FIELDS member of mapper - Use base EntryCollection init in ES collection - Docstrings; add a '.deserialize()' method to mapper - Add classpropertys to mapper docstring for mkdocs Co-authored-by: Markus Scheidgen <markus1978@users.noreply.github.com> Co-authored-by: Johan Bergsma <JPBergsma@users.noreply.github.com> Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
- Skip unknown length test for mongomock, temporarily
444b1b6
to
2776057
Compare
Co-authored-by: Johan Bergsma <JPBergsma@users.noreply.github.com>
ec8318f
to
eb69f62
Compare
Hi @JPBergsma, I have rebased your final suggestions into one commit eb69f62. Thanks again for the thorough review, it would be great to merge this with enough time to prepare tutorials for the workshop! |
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 think you have handled all my comments, so I think you can merge it with the main branch.
This PR lays the groundwork for dealing with some outstanding issues:
None
for "unknown unknown" properties (e.g._other_implementation_band_gap
) (closes Missing optional fields are not returned as null when requested with response_fields #516) and the raising of errors for "known unknown" properties in filters (e.g.unprefixed_optimade_field
) (closes Respect responses for unknown properties #263).MongoDBRemoved, as this is actually a mongomock bug$size: 1
queries match non-existent fields (closes mongomock $size queries match all non-array fields for {$size: 1}, even nulls #807)It does so by:
Quantity
classes defined for elasticsearch.ENTRY_RESOURCE_ATTRIBUTE_CLASS
class attribute to the mappers, such that model schemas can be accessed.Still to-do:
_exmpl2_band_gap
does not fail for provider_exmpl1_
.response_fields
None
if present inresponse_fields
response_fields
(e.g.fractional_coordinates
)_exmpl_test
)null
)