Skip to content

New cf.read kwarg enabling CDL string input#227

Merged
sadielbartholomew merged 10 commits intoNCAS-CMS:masterfrom
sadielbartholomew:read-cdl-from-string
Jul 20, 2021
Merged

New cf.read kwarg enabling CDL string input#227
sadielbartholomew merged 10 commits intoNCAS-CMS:masterfrom
sadielbartholomew:read-cdl-from-string

Conversation

@sadielbartholomew
Copy link
Copy Markdown
Member

Close #171 using the (same) new keyword as suggested in the opening comment of that tagged Issue and in doing so, also improve the error handling for invalid CDL inputs generally.

Note that the API update with the new cdl_string keyword argument is (admittedly) a little clunky, given the core (and only non-keyword) argument is named files and always accepts file locations other than with this new keyword set to True where it then changes to instead accept a string of CDL input (though strictly OPenDAP URLs are also possible to already break the general pattern too, so maybe it is not so bad). However, the only alternative in practice would be to create a whole new function, cf.read_from_string or similar, for the case of reading from a string, only possible with CDL anyway.

So, the approach implemented in this PR to enable the feature of #171 is the most reasonable, and certainly "good enough", I think?

@sadielbartholomew sadielbartholomew added the enhancement New feature or request label Jul 7, 2021
@sadielbartholomew sadielbartholomew self-assigned this Jul 7, 2021
@sadielbartholomew sadielbartholomew changed the title New cf.read keyword argument enabling CDL string input New cf.read kwarg enabling CDL string input Jul 7, 2021
@sadielbartholomew
Copy link
Copy Markdown
Member Author

@davidhassell as you are aware I am largely working on ES-DOC at the moment but I have been addressing some small "low-hanging fruit" items from the issue trackers on the CF libraries, such as this, as a break here and there.

This one is simple enough in implementation but I thought it would be good to check you are happy with the approach of the new kwarg. I am happy to change the approach if you have a better idea to implement the feature of #171!

Copy link
Copy Markdown
Collaborator

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

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

Hi Sadie,

Thanks - I think the approach is sound - I have made a few comments on the structure.

it would be good to copy behaviour to cfdm, too, which we might want to do in parallel, e.h. because some of the error checking can be moved to there.

@sadielbartholomew
Copy link
Copy Markdown
Member Author

I did not realise we were waiting on a merge conflict here until now 😬. I have just resolved it.

All feedback should have been addressed, and all tests are passing locally, so I am now opening and closing the PR to trigger the CI jobs...

@sadielbartholomew
Copy link
Copy Markdown
Member Author

Many of the jobs appeared to be hanging (taking much longer than plausible to complete), so I have cancelled them to save on Actions resource. I will investigate the hanging later. As the one job running the test suite to complete passed, along with the linting job, on top of local passes all-round, I am satisfied the PR is sound.

Thanks for your careful review @davidhassell. I believe I have addressed all of your feedback. Since you are on leave and won't see this until next week at the earliest, I will assume all is good and merge this, but any further or retrospective feedback or issues emerging can be addressed in future commits.

@sadielbartholomew sadielbartholomew merged commit 53dba98 into NCAS-CMS:master Jul 20, 2021
@sadielbartholomew sadielbartholomew deleted the read-cdl-from-string branch July 20, 2021 02:42
@davidhassell davidhassell added this to the 3.11.0 milestone Sep 30, 2021
@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

dataset read Relating to reading datasets enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Means to read-in fields from CDL as a string input

2 participants