-
Notifications
You must be signed in to change notification settings - Fork 300
Provide utility function "file_is_newer_than" for results caching. #787
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
lib/iris/io/__init__.py
Outdated
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.
The is overly complex. Neither of the other functions that use this function make use of the keys.
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.
Whoops I hadn't spotted that.
It was used in the original caller before I split this out of it, but only for the error handling --which is now contained here.
Will fix...
|
Thanks @rhattersley. That really is a lot nicer all round. |
|
NOTE: I just realised, this really needs a "what's new" entry. |
Fixed. Think everything so far is addressed now. |
lib/iris/tests/test_util.py
Outdated
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.
Would you mind moving these tests to the new unit test structure, i.e. into: lib/iris/tests/unit/util/test_file_is_newer_than.py. At which point you reconsider adjusting the class/method groupings to take advantage of the new focus.
|
I've made a couple of minor suggestions but essentially this is a top-quality submission. Thanks @pp-mo 😄 |
lib/iris/tests/test_load.py
Outdated
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.
This would normally be done with the self.assertRaises context manager. See the unittest docs for an example which makes further assertions about the exception raised.
|
Sorry @pp-mo - I didn't spot the modifications to test_load.py before. (Or it might not have been there.) |
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.
As with the changes to test_load.py, this (and the other instances of this idiom in this file) would be better handled with the self.assertRaises context manager.
Yes, I really should have tried harder with that: This way is much nicer. |
Provide utility function "file_is_newer_than" for results caching.
A highly simplified approach to the problems presented by automatic load pickling, (which was recently tried + abandoned for lack of agreement or insuperable problems ?)
The idea is to help the user manage their own results cacheing,
for example ...
The key object of this approach is to have the user take responsibility for all the problems that make an automated approach too difficult and dodgy! Hopefully the docstrings explain the problems + caveats well enough for this.