Merge lp:~stefanor/wadllib/datetime-924240 into lp:wadllib

Proposed by Stefano Rivera
Status: Merged
Approved by: Richard Harding
Approved revision: 28
Merged at revision: 28
Proposed branch: lp:~stefanor/wadllib/datetime-924240
Merge into: lp:wadllib
Diff against target: 19 lines (+2/-7)
1 file modified
src/wadllib/application.py (+2/-7)
To merge this branch: bzr merge lp:~stefanor/wadllib/datetime-924240
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Richard Harding code* Approve
Review via email: mp+97099@code.launchpad.net

Description of the change

I don't understand the purpose of the roundtrippnig through ET in _from_string. More to the point, it claims to track namespaces, but clearly doesn't correctly, causing LP: #924240.

My not understanding, is entirely possibly due to me not knowing this code at all, but the change fixes the issue for me, and the tests still pass under python 2 & 3.

To post a comment you must log in.
Revision history for this message
Richard Harding (rharding) wrote :

So I'm working on getting a test case for this and so I understand how this stuff works. I've got the start of a test case, but it passes with both the old code and your update.

So I'm wondering if maybe the test data in the wadllib isn't up to date. The JSON file that houses the tested data has date strings in the format

"date_created": "2005-06-06T08:59:51.561685+00:00"

And I wonder if that's not true for what you're getting.

Can you please get a copy of the json you're testing against so I can try to create a test that breaks as you're seeing it to make sure this doesn't become an issue in the future.

This is a paste of the test I've started to try to fire up:
https://pastebin.canonical.com/62637/

review: Needs Information
Revision history for this message
Richard Harding (rharding) wrote :

I've been unable to break this, but this change, regardless doesn't break any wadllib tests and makes the code simpler so approving.

review: Approve (code*)
Revision history for this message
j.c.sackett (jcsackett) wrote :

I don't see anything wrong with this change. Please do provide the data requested by Rick so he can get tests in, thanks.

review: Approve
Revision history for this message
Stefano Rivera (stefanor) wrote :

We discussed it on IRC. I didn't have any particular test data, was just looking at datetimes with launchpadlib. But I'll have a look and see if I can prepare a test.

Revision history for this message
Richard Harding (rharding) wrote :

That's ok Stefano, we chatted in irc and I think the test would require some integration between not just the wadllib, but the api library as well. We ok on this change.

Revision history for this message
j.c.sackett (jcsackett) wrote :

> We discussed it on IRC. I didn't have any particular test data, was just
> looking at datetimes with launchpadlib. But I'll have a look and see if I can
> prepare a test.

Ah, thanks for the update. As Rick said, nevermind on the test, then. :-)

Revision history for this message
Stefano Rivera (stefanor) wrote :

phew :P

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/wadllib/application.py'
2--- src/wadllib/application.py 2012-01-27 18:43:16 +0000
3+++ src/wadllib/application.py 2012-03-12 20:47:20 +0000
4@@ -1104,13 +1104,8 @@
5 return ET.ElementTree(root)
6
7 def _from_string(self, markup):
8- """Turns markup into a document.
9-
10- Just a wrapper around ElementTree which keeps track of namespaces.
11- """
12- # We're using the ET fromstring/tostring to help make sure we maintain
13- # py2/3 compatiblity and keeping things in Unicode.
14- return self._from_stream(BytesIO(ET.tostring(ET.fromstring(markup))))
15+ """Turns markup into a document."""
16+ return self._from_stream(BytesIO(markup))
17
18 def get_resource_type(self, resource_type_url):
19 """Retrieve a resource type by the URL of its description."""

Subscribers

People subscribed via source and target branches