Skip to content

move sort_xml_attrs #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jamesp merged 3 commits into from
Feb 7, 2021
Merged

move sort_xml_attrs #1

jamesp merged 3 commits into from
Feb 7, 2021

Conversation

@bjlittle
Copy link

@bjlittle bjlittle commented Feb 6, 2021

🚀 Pull Request

Description

@jamesp I love you're approach - very elegant 😉

This is a minor compromise to get around creating a iris/xml.py, which would be part of the public API and we don't really want to expose it. However, it still dodges the troublesome circular imports issue.

I'm happy doing an import within the localised scope of the iris.cube.Cube.xml and iris.cube.CubeList.xml methods, as this functionality is only ever really called in anger as part of testing, and there shouldn't be any detrimental performance hit in doing so.

Let my know what you think 👍

Consult Iris pull request check list

@bjlittle
Copy link
Author

bjlittle commented Feb 6, 2021 •
edited
Loading

@jamesp Had a second thought, which I'm even more comfortable with, which is to make sort_xml_attrs a classmethod of Cube.

Seems like another reasonable approach to take rather than polluting iris.__init__ with a _sort_xml_attrs function. Rather, confine _sort_xml_attrs to the namespace of the Cube, and it's still accessible without a Cube instance for CubeList and assertXMLElement... and it completely avoids the dangers of circular imports.

@bjlittle
Copy link
Author

bjlittle commented Feb 7, 2021

Also, just discovered that iris-grib suffers from the same issues in Python 3.8 as it also has cml tests.

However, the fix for iris will also fix iris-grib, as it borrows the testing infra-structure from iris.

That said, this bolsters the argument for maintaining the legacy sorted order for xml within iris - it's technical debt we need to be aware of and carry forwards

@jamesp jamesp merged commit 7d92803 into jamesp:iris-py38 Feb 7, 2021
@jamesp
Copy link
Owner

jamesp commented Feb 8, 2021

I'm not sure how elegant this is, but it works!

I'll tidy this up and put it back to you to update your PR

@bjlittle bjlittle deleted the move-sort-xml branch February 15, 2021 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bjlittle @jamesp