Skip to content

Conversation

@davidhassell
Copy link
Collaborator

@davidhassell davidhassell commented Sep 9, 2022

Fixes #448

Needs NCAS-CMS/cfdm#216 to work.

@davidhassell
Copy link
Collaborator Author

Needs NCAS-CMS/cfdm#216 to work.

@davidhassell davidhassell changed the title Original filenames Original filenames methods Sep 9, 2022
@davidhassell
Copy link
Collaborator Author

OK - finished adding the bits I realised were missing!

@davidhassell
Copy link
Collaborator Author

Note - some of the .rst files that have been modified in this PR had been, for some reason, reformatted into some sort of sphinx autodoc format, so these few (~6) have also been reinstated to the structured form. e.g. cf.CellMeasure.rst.

@davidhassell davidhassell added the dask Relating to the use of Dask label Sep 22, 2022
Copy link
Member

@sadielbartholomew sadielbartholomew left a 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.

def files(self):
"""Deprecated at version 3.4.0, use method `get_filenames`
instead."""
"""Deprecated at version 3.4.0, consider usin method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Deprecated at version 3.4.0, consider usin method
"""Deprecated at version 3.4.0, consider using method

@davidhassell
Copy link
Collaborator Author

Implementation now inherited from cfdm - see NCAS-CMS/cfdm#216 (comment) for details.

@davidhassell davidhassell marked this pull request as draft October 6, 2022 21:20
@davidhassell davidhassell marked this pull request as ready for review October 6, 2022 21:21
@sadielbartholomew
Copy link
Member

(Just resolved the trivial change log merge conflicts so I can re-review more quickly, which I am about to do now...)

Copy link
Member

@sadielbartholomew sadielbartholomew left a 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

@davidhassell davidhassell merged commit f64ec3e into NCAS-CMS:master Oct 7, 2022
@davidhassell davidhassell deleted the original-filenames branch November 15, 2022 09:30
@davidhassell davidhassell added this to the 3.14.0 milestone Nov 15, 2022
@davidhassell davidhassell added the dataset read Relating to reading datasets label Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dask Relating to the use of Dask dataset read Relating to reading datasets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New "original filenames" methods

2 participants