Skip to content
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.

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

fix etree parse on literal string #56

Merged
merged 2 commits into from
Dec 9, 2020
Merged

fix etree parse on literal string #56

merged 2 commits into from
Dec 9, 2020

Conversation

tlambert03
Copy link
Owner

@tlambert03 tlambert03 commented Nov 17, 2020

This should fix the issue that @JacksonMaxfield had in #50 (comment) ... wherein ElementTree.parse() is looking for a path name or file-like object, but receiving a literal xml string (when parsing structured_annotations).

cc @jmuhlich

@tlambert03
Copy link
Owner Author

tlambert03 commented Nov 17, 2020

actually... @jmuhlich, I'm getting an error in roundtrips on python 3.7 now (not related to this PR ... it's even on master) with the C14NWriterTarget backport:

testing/test_model.py:117:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
testing/test_model.py:113: in canonicalize
    xml_out = util.canonicalize(xml_out, strip_text=True)
testing/util.py:36: in canonicalize
    parser.feed(xml_data)
testing/util.py:205: in start
    self._start(tag, attrs, new_namespaces)
testing/util.py:234: in _start
    n: parse_qname(n) for n in sorted(qnames, key=lambda n: n.split("}", 1))
testing/util.py:234: in <dictcomp>
    n: parse_qname(n) for n in sorted(qnames, key=lambda n: n.split("}", 1))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <util.C14NWriterTarget object at 0x1110322d0>, qname = '{http://www.openmicroscopy.org/Schemas/OME/2016-06}OME'
uri = 'http://www.openmicroscopy.org/Schemas/OME/2016-06'

    def _qname(self, qname, uri=None):
        if uri is None:
            uri, tag = qname[1:].rsplit("}", 1) if qname[:1] == "{" else ("", qname)
        else:
            tag = qname

        prefixes_seen = set()
        for u, prefix in self._iter_namespaces(self._declared_ns_stack):
            if u == uri and prefix not in prefixes_seen:
                return f"{prefix}:{tag}" if prefix else tag, tag, uri
            prefixes_seen.add(prefix)

        # Not declared yet => add new declaration.
        if self._rewrite_prefixes:
            if uri in self._prefix_map:
                prefix = self._prefix_map[uri]
            else:
                prefix = self._prefix_map[uri] = f"n{len(self._prefix_map)}"
            self._declared_ns_stack[-1].append((uri, prefix))
            return f"{prefix}:{tag}", tag, uri

        if not uri and "" not in prefixes_seen:
            # No default namespace declared => no prefix needed.
            return tag, tag, uri

        for u, prefix in self._iter_namespaces(self._ns_stack):
            if u == uri:
                self._declared_ns_stack[-1].append((uri, prefix))
                return f"{prefix}:{tag}" if prefix else tag, tag, uri

>       raise ValueError(f'Namespace "{uri}" is not declared in scope')
E       ValueError: Namespace "http://www.openmicroscopy.org/Schemas/OME/2016-06" is not declared in scope

any thoughts?

@evamaxfield
Copy link
Contributor

Just wanted to comment here and say I just tried out this branch and it works for my file. I figured I would give a shot at understanding the broken tests. Interestingly, the tests work just fine with pytest testing/ but fail on tox. Will keep investigating.

@tlambert03
Copy link
Owner Author

tlambert03 commented Dec 1, 2020

Oh darn, sorry I didn't push this through faster. Thanks for trying it out, I'll make sure to finish it up this week one way or another

@evamaxfield
Copy link
Contributor

No worries on timing. I was busy as well and just started working again. Thanks for all existing work on this haha 🙂

@tlambert03
Copy link
Owner Author

fixed the tests error. had to do with the "imported" xml.etree.ElementTree._namespace_map diverging from the actual one. merging now

@tlambert03 tlambert03 merged commit 0a37d5c into master Dec 9, 2020
@tlambert03 tlambert03 deleted the fix-etree-parse branch December 9, 2020 19:59
@evamaxfield
Copy link
Contributor

evamaxfield commented Dec 9, 2020

Thanks for the patch!

@tlambert03
Copy link
Owner Author

@JacksonMaxfield, if a new bug-fix release would make your life easier when testing this out, let me know and I'll push one

@evamaxfield
Copy link
Contributor

@JacksonMaxfield, if a new bug-fix release would make your life easier when testing this out, let me know and I'll push one

I will keep testing with the latest master branch now. I may have a PR of my own to this lib, still debugging though. If I do have a PR would be best to patch out together probably.

@tlambert03
Copy link
Owner Author

lol ... oops, just pushed 0.2.1, figuring we wouldn't have anything soon. No worries though! If you have something, that's great! no problem releasing again soon

@evamaxfield
Copy link
Contributor

Hey @tlambert03 I am also having issues where my pytest stuff passes but tox doesn't. Specifcally, it breaks on ElementTree.tostring, since you had the same issue I looked through commits and see you single forced name spaced ElementTree. on all functions and such. I tried the same but can't get it to work - any ideas?

@tlambert03
Copy link
Owner Author

can I see the full trace? Does it fail in tox in all python versions? or just in one of them? We vendored some of ElementTree for testing purposes on py3.7, and there may still be some hiccups in the approach.

@evamaxfield
Copy link
Contributor

Wow. That was it. I kept interrupting the tests after they failed on py37 and didn't let them run through py38 or py39. From xml docs I now see that the xml_declaration parameter is only available on 3.8+. Remove that kwarg fixed it. My bad!

Thanks for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants