Ignore XML namespaces in xsi:type #323
Conversation
Since this is a rather ugly hack, a few words of background: It turns out that astropy.utils.xml completely discards all namespace information. That you can parse Registry/capability documents with it in the first place is just because we only have local elements with elementFormDefault=unqualified. Ah well. Given that, we can either do XML processing ourselves, properly managing namespaces (which wouldn't be a terrible amount of work). Or we ignore namespaces and namespace prefixes, too. Since I can't think of anywhere that would be trouble in the current VO, this commit opts for the second option.
Codecov Report
@@ Coverage Diff @@
## main #323 +/- ##
==========================================
- Coverage 75.53% 75.46% -0.07%
==========================================
Files 44 44
Lines 5137 5123 -14
==========================================
- Hits 3880 3866 -14
Misses 1257 1257
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
I've looked at where the coverage machinery thinks we're losing coverage, and if I read its output right, it's a few setters and a raise that the machinery thinks we're losing. For all these, I've set a breakpoint in the correponding code of the current main branch, and it didn't fire when ran a pytest; I've also not found which test might make the machinery think it's covered in current main (but I've not looked hard). So... until someone convinces me otherwise, I claim the loss of test coverage is not real. |
|
So, umm... who'll merge this? Do we have a policy? |
I can do that. We don't have a policy and I usually prefer a second pair of eyes. But if you say that this is ready, I will merge it. |
Maybe this should be eventually upstreamed, too? (Of course that is totally independent of this PR, as even if upstreamed there should be a pyvo solution until versions are being caught up) |
|
On Mon, May 16, 2022 at 09:09:41AM -0700, Adrian wrote:
> So, umm... who'll merge this? Do we have a policy?
I can do that. We don't have a policy and I usually prefer a second
pair of eyes. But if you say that this is ready, I will merge it.
I don't have a good idea for how to do further validation, and it
solves the MAST and ESO problems; so: yeah, I don't think there's any
point waiting a lot longer.
And perhaps [let me muse here briefly rather than open a bug for
this] we ought to think about a policy. You see, I'd still like to
see PR #289 merged, sooner rather than later: It *is* quite a
mouthful, and I'd appreciate if it got a chance to two to show
problems before it ends up in a release. And I'm not sure what I'd
need to do to make that happen.
One *could* argue that the RegTAP 1.1 constraints ought to be
annotated as per #309 -- do we? If so, can I do something to get
#309 merged?
|
|
On Mon, May 16, 2022 at 10:09:00AM -0700, Brigitta Sipőcz wrote:
> Rationale: It turns out that astropy.utils.xml completely
> discards all namespace information. That you can parse
> Registry/capability documents with it in the first place is just
> because we only have local elements with
> elementFormDefault=unqualified. Ah well.
Maybe this should be eventually upstreamed, too? (Of course that is
totally independent of this PR, as even if upstreamed there should
be a pyvo solution until versions are being caught up)
I'd say no. The right way to deal with this is to teach namespace
handling to astropy and ideally rationalise it while we're on it
(i.e., canonical prefixes for known namespaces). But I'd really make
that dependent on whether the VO-DML folks get their act together.
In that case, namespace handling will become relevant to a VOTable
parser and it makes sense to burden astropy with it.
If VO-DML annotation doesn't happen or happens within VOTable rather
than in an external schema, I'd say we ought to do our own,
namespace-aware XML handling on the long run. Let's look again in
four years. Until then, the present makeshift solution ought to be
just fine.
|
Is #289 ready? For #309 you could help with a review. I did review it and have no major concerns but it will be nice if someone else could do it too. |
If there are no other takers I'll try to give it a go through later this week, I need to get used to type hinted code anyway (not sure we need it here at all, but that's not my call). |
|
On Tue, May 17, 2022 at 04:32:33PM -0700, Adrian wrote:
Is #289 ready?
From my PoV, yes: I have no immediate plans to change it, and it's
seen some exercise in practice.
For #309 you could help with a review. I did review it and have no
major concerns but it will be nice if someone else could do it too.
Since Brigitta volunteers, I'm happily standing back... Feel free to
poke me if you think there should be a fourth pair of eyes.
|
|
Ummm... can I bump this? It'd be great if it could be merged before it grows conflicts... |
This PR addresses bug #257 in the lamest possible way: It just ignores namespace prefixes on xsi:type-s.
Rationale: It turns out that astropy.utils.xml completely discards all namespace information. That you can parse Registry/capability documents with it in the first place is just because we only have local elements with elementFormDefault=unqualified. Ah well.
Given that, we can either do XML processing ourselves, properly managing namespaces. Or we ignore namespaces and namespace prefixes, too. Since I can't think of anywhere that would be trouble in the current VO, this commit opts for the second option.
I suppose I'd scupper astropy.utils.xml for registry documents and have our own XML parser (which isn't a big deal in this context) unless I'd expect that astropy.utils.xml will deal with namespaces in some way in the near future, as namespaces in VOTable will probably become relevant when VO-DML annotation enters VOTables. If that happens, we ought to revise ignoring namespaces here. If not... well, we can always employ our own namespace-aware XML parser later.