-
Notifications
You must be signed in to change notification settings - Fork 177
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
Cleanup #1406
Cleanup #1406
Conversation
All looks good on a quick glance. Will review properly when it's ready - thanks for all this, Ariana. |
c59b138
to
bf1faf3
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.
Looking forward to getting rid of those DatasetTypes!
datacube/model/__init__.py
Outdated
@@ -21,9 +21,11 @@ | |||
schema_validated, DocReader | |||
from datacube.index.eo3 import is_doc_eo3 | |||
from .fields import Field, get_dataset_fields | |||
from ._base import Range, ranges_overlap # noqa: F401 | |||
from ._base import Range |
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 ranges_overlap is here so you can from datacube.model import ranges_overlap
.
You can get rid of the flake8 ignore by defining an __all__
datacube/utils/documents.py
Outdated
except (AttributeError, KeyError, ValueError): | ||
continue | ||
return fields | ||
return {name: field.extract(self.doc) |
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.
Are there implications for not catching those errors? It feels like a risky thing to move, but it seems things would have to be pretty broken elsewhere for any of those errors to occur.
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.
There'd have to be a mismatch between the MetadataType definition and the Dataset's metadata document. If that doesn't already cause other problems elsewhere, it might be better to throw an exception or at least log a warning instead of letting it silently slip past.
41285d6
to
d8e8057
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop-1.9 #1406 +/- ##
===============================================
+ Coverage 91.95% 91.97% +0.01%
===============================================
Files 135 135
Lines 15085 15070 -15
===============================================
- Hits 13872 13860 -12
+ Misses 1213 1210 -3
☔ View full report in Codecov by Sentry. |
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.
All makes sense to me. Thanks again, Ariana.
Reason for this pull request
The datacube-core code could use some cleanup. I'm starting off with some small/easy wins, primarily focused on removing/consolidating duplicated logic and making code more pythonic and concise.
Proposed changes
Merged
get_doc_offset
andget_doc_offset_safe
Prettified DocReader properties, and added
doc
attributeAdded a few deprecation warnings with the deprecat library
Closes #xxxx
Tests added / passed
Fully documented, including
docs/about/whats_new.rst
for all changes📚 Documentation preview 📚: https://datacube-core--1406.org.readthedocs.build/en/1406/