Original filenames methods

[davidhassell]

Fixes #215

[codecov]

Codecov Report

Merging #216 (3c7a499) into master (cbe7009) will increase coverage by 0.09%.
The diff coverage is 96.65%.

❗ Current head 3c7a499 differs from pull request most recent head 78f323f. Consider uploading reports for the commit 78f323f to get more accurate results

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
+ Coverage   87.55%   87.64%   +0.09%     
==========================================
  Files         122      123       +1     
  Lines       12470    12608     +138     
==========================================
+ Hits        10917    11049     +132     
- Misses       1553     1559       +6     

Flag	Coverage Δ
unittests	87.64% <96.65%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cfdm/docstring/docstring.py	100.00% <ø> (ø)
cfdm/mixin/properties.py	89.26% <ø> (ø)
cfdm/data/data.py	85.59% <89.75%> (+0.24%)	⬆️
cfdm/mixin/propertiesdatabounds.py	83.24% <90.91%> (-0.04%)	⬇️
cfdm/auxiliarycoordinate.py	100.00% <100.00%> (ø)
cfdm/bounds.py	90.00% <100.00%> (+0.21%)	⬆️
cfdm/cellmeasure.py	84.62% <100.00%> (+0.25%)	⬆️
cfdm/cfdmimplementation.py	83.11% <100.00%> (+0.37%)	⬆️
cfdm/coordinatereference.py	90.00% <100.00%> (+0.08%)	⬆️
cfdm/count.py	100.00% <100.00%> (ø)
... and 18 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[davidhassell]

OK - finished adding the bits I realised were missing!

[sadielbartholomew]

Something to indicate explicitly that only one of the three keyword parameters can/should be set, i.e. not any two or all three, might be useful, even if it should be clear enough from the descriptions.

[davidhassell]

I'm happy leaving this to be trapped in the code:

                raise ValueError(
                    "Can't set the 'define' and 'update' parameters "
                    "at the same time"

                raise ValueError(
                    "Can't set the 'clear' parameter with either of the "
                    "'define' and 'update' parameters"
                )

Is that OK with you? Happy to doc it if you prefer.

[sadielbartholomew]

I'm inclined to say I think it should also be documented, even if that is to very concisely indicate as such, but it's your call!

[davidhassell]

OK - will do. I'll hold off implementing it until we're agreed on where we stand on NCAS-CMS/cf-python#448 (comment)

[sadielbartholomew]

(To finalise the review started last week, with inline comments as raised above.)

Overall, I agree with the approach proposed in #215 to deal with the underlying deficiencies discussed in NCAS-CMS/cf-python#365, as applied here. It seems like a sensible way to deal with the issue at hand. However, I did have some questions regarding the approach so please see NCAS-CMS/cf-python#448 (comment).

As for the implementation, there are quite a few minor docs issues e.g. typos, as raised (and expected given the nature of the various new methods added and adaptations to the rest of the code required!), but otherwise it is sound (assuming the question referenced above is addressed), with an especially good job on the new tests which are nice and rigorous.

Please merge when ready. Thanks.

[davidhassell]

Change of tack

Following a discussion with @sadielbartholomew on question raised at NCAS-CMS/cf-python#448, the code has been refactored such that:

• defining, updating and clearing original filenames can only be done with the _original_filenames methods, which are available on all relevant classes
• original filename access is by the get_original_filenames methods, which are available on all relevant classes
• The on-demand link between a construct's original filenames and those of it's Data is broken.
• The original filenames are now stored on the constructs rather than their data (because some constructs don't have data!)
• It is, however, still convenient in cfdm.read to get original files from the data during the one-off initialisation.

Ready for a new review!

[sadielbartholomew]

The changelog needs updating on account of the new approach in #216 (comment) as raised inline, but overall the new commits adapt the PR to implement that precise approach, and do so in a very elegant manner (nice use of a new mixin class)! 💯 Thanks and please merge when ready.