- 
                Notifications
    You must be signed in to change notification settings 
- Fork 39
          feat: add dynamic has_resources property to DatasetDefinition
          #1064
        
          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
Conversation
for more information, see https://pre-commit.ci
| Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff            @@
##              main     #1064   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           90        91    +1     
  Lines         3986      4028   +42     
  Branches       708       712    +4     
=========================================
+ Hits          3986      4028   +42     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
# Conflicts: # src/pymovements/dataset/dataset_definition.py
has_resources field to DatasetDefinitionhas_resources property to DatasetDefinition
      for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
couple comments/questions.
for more information, see https://pre-commit.ci
# Conflicts: # src/pymovements/dataset/_utils/_resources.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| ah I was confused about | not changed but I guess 3.9 **** | 
Description
This PR adds
DatasetDefinition.has_resourcesas a getter property that is inferred dynamically on each call.The
has_resourcesproperty functions both asbool:and as an indexable object:
This is achieved by a helper class
_HasResourcesIndexerthat is forwarded by the getter property.The contribution has two shortcomings that should be improved on in follow-up PRs:
has_resources. This could be resolved by implementing aDatasetResourcesclass that would be the type ofDatasetDefintion.resources.DatasetDefintion.has_resourceswould then simply callDatasetDefintion.resources.has_resources.DatasetDefinition.has_files, as its usage indataset_files.pyhas different semantics: it does not refer to downloadable resources, but to local resources (which could have been downloaded before). Moving resource-dependent attributes to each resource would make a full replacement possible.Both solutions to the shortcomings are addressed in #1120.
Implemented changes
_HasResourcesIndexerto makehas_resourcesindexablehas_resources()property and forward_HasResourceIndexerhas_filestohas_resourcesinpublic_dataset_processing_test.pyType of change
How Has This Been Tested?
Context
related issues:
requires:
DatasetDefinition.to_dict()#1088Checklist: