-
Notifications
You must be signed in to change notification settings - Fork 23
Original filenames methods #449
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
Original filenames methods #449
Conversation
|
Needs NCAS-CMS/cfdm#216 to work. |
|
OK - finished adding the bits I realised were missing! |
|
Note - some of the |
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 with regards to resolving #448 on top of the upstream implementation in place for cfdm, though I raised one minor comment in-line and I do have some questions regarding the approach so please see #448 (comment).
(I do wonder if it might have been better to raise this against lama-to-dask, but since both branches are fairly syncronised at the moment it should be OK on master for checking it works as we've done now, and when we merge the former into the latter the merge conflicts should be OK, assuming you don't see a reason why they might not be that I don't!)
Please merge when ready. Thanks.
cf/data/mixin/deprecations.py
Outdated
| def files(self): | ||
| """Deprecated at version 3.4.0, use method `get_filenames` | ||
| instead.""" | ||
| """Deprecated at version 3.4.0, consider usin method |
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.
| """Deprecated at version 3.4.0, consider usin method | |
| """Deprecated at version 3.4.0, consider using method |
|
Implementation now inherited from |
|
(Just resolved the trivial change log merge conflicts so I can re-review more quickly, which I am about to do now...) |
sadielbartholomew
left a comment
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, re-review accounting for the change of approach covered in #449 (comment) and a quick sanity check on the overall PR state to boot. Thanks, please merge!
| available. Consider using the `get_original_filenames` method | ||
| instead. | ||
| .. note:: Might get re-instated in a later version. |
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.
😆
Fixes #448
Needs NCAS-CMS/cfdm#216 to work.