-
Notifications
You must be signed in to change notification settings - Fork 45
feat: accumulation from grib-index #317
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
base: main
Are you sure you want to change the base?
Conversation
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 good ! Only one small comment.
assert not kwargs | ||
raise NotImplementedError("requests not implemented") | ||
|
||
if kwargs: |
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.
Could we briefly explain why requests and kwargs are incompatible ?
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.
Do we have tests to check the new behaviour?
indexdb=use_grib_index, | ||
flavour=None, | ||
requests=requests, | ||
request_already_using_valid_datetime=True, |
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.
request_already_using_valid_datetime
isn't actually changing the behaviour of the code, as far as I can tell
|
||
if requests is not None: | ||
assert not kwargs | ||
raise NotImplementedError("requests not implemented") |
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.
If we're not using it, why add it?
raise NotImplementedError("requests not implemented") | ||
|
||
if kwargs: | ||
assert not requests |
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.
Because request_already_using_valid_datetime=True
in the accumulation source, this will fail if requests
is truthy – is it better to capture that error at that level rather than here? (I don't mind - just curious)
---------- | ||
dates : List[Any] | ||
List of dates to retrieve data for. | ||
request_already_using_valid_datetime : bool, optional |
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.
request_already_using_valid_datetime
is set to True
in the accumulation code, has a default of False
here - but there's no switch that changes the behaviour of the code. This feels incorrect to me
Description
Type of Change
Issue Number
Code Compatibility
Code Performance and Testing
Dependencies
Documentation
Additional Notes