New cf.read kwarg enabling CDL string input#227
New cf.read kwarg enabling CDL string input#227sadielbartholomew merged 10 commits intoNCAS-CMS:masterfrom
Conversation
|
@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! |
davidhassell
left a comment
There was a problem hiding this comment.
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.
|
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... |
|
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. |
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_stringkeyword argument is (admittedly) a little clunky, given the core (and only non-keyword) argument is namedfilesand always accepts file locations other than with this new keyword set toTruewhere 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_stringor 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?