Skip to content

centralize some XML helper functions#1624

Open
woutdenolf wants to merge 2 commits intomainfrom
1623-dev_tools-xml-tag-parsing
Open

centralize some XML helper functions#1624
woutdenolf wants to merge 2 commits intomainfrom
1623-dev_tools-xml-tag-parsing

Conversation

@woutdenolf
Copy link
Contributor

@woutdenolf woutdenolf commented Mar 15, 2026 •
edited
Loading

Closes #1623

I checked that this does not introduce regressions

diff -ru --exclude='*.js' build/manual/build/html keep/manual/build/html

@woutdenolf woutdenolf linked an issue Mar 15, 2026 that may be closed by this pull request
dev_tools: XML tag parsing #1623
Open
woutdenolf
woutdenolf commented Mar 15, 2026
View reviewed changes
dev_tools/utils/xml_utils.py
except OSError:
# Not sure this is still necessary
with open(normalized_path, "r") as f:
return ET.parse(f).getroot()
Copy link
Contributor Author

@woutdenolf woutdenolf Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced by @sanbrock in https://github.com/nexusformat/definitions/blame/f6791c01c9ea9f531ef0e04e14ae737a137fa091/dev_tools/utils/nxdl_utils.py#L743

Not sure why.

Copy link
Contributor

@PeterC-DLS PeterC-DLS Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither! The source code does open with "rb" which is a difference across platforms but that shouldn't matter for XML...

Copy link
Contributor

@PeterC-DLS PeterC-DLS Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend removing it unless @sanbrock recalls why.

@woutdenolf woutdenolf requested a review from PeterC-DLS March 15, 2026 18:50
@woutdenolf woutdenolf force-pushed the 1623-dev_tools-xml-tag-parsing branch from 96d7ec8 to 8881640 Compare March 15, 2026 18:52
PeterC-DLS
PeterC-DLS reviewed Mar 16, 2026
View reviewed changes
PeterC-DLS
PeterC-DLS approved these changes Mar 16, 2026
View reviewed changes
Copy link
Contributor

@PeterC-DLS PeterC-DLS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM subject to Sandor's response

@lukaspie
Copy link
Contributor

lukaspie commented Mar 17, 2026

@woutdenolf thanks for this effort.

We are currently using functions from these dev_tools directly in some of our tools in https://github.com/FAIRmat-NFDI/pynxtools. That's also the reason why some function mangle hdf5 and xml logic.

We are eventually planning to move away from this (and then there could be further cleanup done here), but that would not be possible right away. Therefore, I would like to check that these tools still work with your changes from this branch. Could you hold of with merging while I test this? Thanks!

@woutdenolf woutdenolf mentioned this pull request Mar 18, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PeterC-DLS PeterC-DLS PeterC-DLS approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dev_tools: XML tag parsing

3 participants

@woutdenolf @lukaspie @PeterC-DLS