Iris py38

[bjlittle]

🚀 Pull Request

Description

This PR extends the iris testing to cover python 3.8.x.

In order to provision this testing infra-structure, work was required on cml generation, as xml.dom.minidom has changed from 3.8 to preserve xml element attribute assignment order - most likely due to python dictionaries now preserving order.

In addition to this a minor fix was required for the doc-tests, as a namedtuple._todict() now returns a dict rather than an OrderedDict.

Note that, all the CI documentation is performed soley for python 3.8 i.e.,

• the documentation gallary
• the doc-tests
• the documentation link-check

---

Consult Iris pull request check list

[jamesp]

Started a review, this is a great way to get stuck into understanding the codebase! Will give it some more time later, a couple of comments/questions inline below.

[jamesp]

is there a reason this method returns a different signature (element, attributes) vs. (element,) depending on the param value split? different use cases?

[jamesp]

The default signature xml_element(doc) => (element,) is different to the default signature of e.g. Coord#xml_element => (element, attributes). The api seems inconsistent throughout this file, is this expected?

[bjlittle]

@jamesp Yes, this is intentional, and I couldn't see a neater way around it.

The issue is being forced by the inheritance hierarchy and the fact that the xml_element method in the final inheritance call chain must return an xml.dom.minidom.Element.

Just be to explicit we have the following:

CellMethod
`-- xml_element()

_DimensionalMetadata
|-- AncillaryVariable
|   |-- CellMeasure
|   |   `-- xml_element()
|   `-- xml_element()
|-- Coord
|   |-- AuxCoord
|   |   `-- xml_element()
|   |-- DimCoord
|   |   `-- xml_element()
|   `-- xml_element()
`-- xml_element()

So AncillaryVariable.xml_element by default requires to return an xml.dom.minidom.Element if it is directly called, otherwise, if AncillaryVariable.xml_element is called indirecty as part of the super in CellMeasure.xml_element, CellMeasure requires AncillaryVariable.xml_element to return (xml.dom.minidom.Element, dict) instead as it still has has to mix-in the measure attribute before finally calling xml.dom.minidom.Element.setAttribute.

Why are we doing this now?

Well, basically, for Python <3.8 the __str__/__repr__ of a xml.dom.minidom.Element would show its elements "for free" in alphabetical order... for Python >=3.8 it now shows its elements in assigned order - which breaks all our cml tests i.e., I'm guessing this is a direct side-effect of dicts behaving more like OrderedDicts.

So the deal is that we need all the attributes (key-values pairs) that we're ever going to set on the xml.dom.minidom.Element all at once before doing so, but sort them by key first, to get the same behaviour pre-3.8.

The fact that CellMeasure is a sub-class of AncillaryVariable is forcing this dual return type behaviour from AncillaryVariable.

This is also the case for Coord, but as this is an abstract base class for AuxCoord and DimCoord it is never called directly (as it's not possible), and so always returns (xml.dom.minidom.Element, dict). This also holds true for _DimensionalMetadata.

Does this help?

[bjlittle]

BTW I'm not wedded to the split kwarg as a name. If you agree on my strategy of doing all this, and the purpose of the split kwarg... then please suggest a rename for it, if you can think of something better.

[bjlittle]

I guess I could be a good citizen here and add all the missing doc-strings for the xml_element methods documenting the args, kwargs and return types...

[bjlittle]

In fact, I'm just going to go ahead an just do that... why not 😉

[bjlittle]

Done

[tkknight]

Overall looks ok to me @bjlittle . I would only add to ensure the docs are updated to include that Python 3.8 is now supported and tested. See the existing on the installing Iris page: https://scitools-iris.readthedocs.io/en/latest/installing.html

Over to you @jamesp to finish your review with @bjlittle. 😃

[bjlittle]

@jamesp I think this is good to go 😀

[jamesp]

@bjlittle ok! 👍

[jamesp]

@bjlittle can you just confirm this line supposed to be deleted? comment below suggests that it is

[bjlittle]

@jamesp Yup, that's fine, and it's a good question...

It's actually redundant, and I only noticed it by working through the inheritance behaviour.

Coord.xml_element calls _DimensionalMetadata.xml_element, and in the super class it performs the following:

        # The values are referred to "points" of a coordinate and "data"
        # otherwise.
        if isinstance(self, Coord):
            values_term = "points"
        else:
            values_term = "data"
        element.setAttribute(values_term, self._xml_array_repr(self._values))

So as it stood, the _DimensionalMetadata was setting the points attribute, then the Coord came along an stomped over it with the same result... seemed like worthwhile trimming that fat whilst we're here.

If it didn't work... we'd have test failures all over the place. So I believe this is fine.

Lovely spot though 🦅 👀

[bjlittle]

@jamesp I've got come conflicts that I need to resolve...