Codecov Report

Merging #104 (7113add) into master (723b5bc) will decrease coverage by 0.04%.
The diff coverage is 95.24%.

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   88.68%   88.65%   -0.03%     
==========================================
  Files         101      101              
  Lines       10008    10050      +42     
==========================================
+ Hits         8875     8909      +34     
- Misses       1133     1141       +8     

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 723b5bc...de4ceb5. Read the comment docs.

Hi @davidhassell, I think that is an excellent solution: minimal code required to implement the inheritance, as per your example snippet, and it can keep the logic loosely coupled:

So cf-python need not know anything about the functionality nor API nor docs from cfdm.

Good thinking.

I was trying to work out whether the __doc__ replacement would work in light of us running the docstring rewriting logic on all of the docstrings (even if they don't themselves have anything that would be replaced by it), but I think it shouldn't be a problem since the docstring rewriting is done via metaclass which should happen before the replacement happens. I could be wrong though, have you checked it works in this respect (if it happened the other way round the replacement might be futile, if that makes sense)?

Thanks. Good point on doc string rewriting. Currently it is not in place for these new classes, but I reckon if we add simply the metaclass to ConstantAccess the magic will just happen ... I'll give it a go. The doc string overwriting is somewhat clunky, so it's nice to get rid of it.

Currently it is not in place for these new classes

Oh, I didn't consider this, so I guess a simple option is just to not apply it to those classes, if it does end up causing issue!

On the topic the atol and rtol docstrings are pretty similar so to enforce consistency they may be a good candidate for one or more common text snippets to process in, though being only two cases it might be overkill.

All good. If you would like review once this is ready just ping me. Thanks.

Will do. I've just remembered, after a bit of tinkering, that the docstring rewriting only happens on methods of the class, rather than the class itself.

It may/may not be trivial to extend it to the __doc__ of the class. For methods, the original method gets copied, the copy's doc string is rewritten and then the new method replaces the old one. This is so that docstring changes do not affect other classes that inherit the same method.

Can we somehow copy the class itself in the same way. I have no idea, but it'll fun(?) finding out.

If this is not easy to fix, I suggest that we implement it as it stands and dwell on the more elegant solution at our leisure.

(I should say that adding the metaclass absolutely did the trick for method docstring rewriting on the rtol class)

Can we somehow copy the class itself in the same way. I have no idea, but it'll fun(?) finding out.

I think that should be do-able via an approach not dissimilar to one used for copying the methods. But I could be wrong. Would not some investigation, Good luck, if you want to try it.

If this is not easy to fix, I suggest that we implement it as it stands and dwell on the more elegant solution at our leisure.

Sounds like a good plan to me. I think the class copying for class docstring re-writing, as above, constitutes another issue entirely, so perhaps we can throw an issue up for it to tackle at a good time.

Class docstring rewriting is a reality . I think it's all there, now, for now.

HI Sadie - I think it's OK to merge now?

HI Sadie - I think it's OK to merge now?

I found a hash symbol lurking, as in my above comment, but otherwise the minor feedback is all addressed and everything is shipshape, so please merge! I'll start reviewing the cf-python follow-on PR this afternoon.

Great - thanks