Merge lp:~leonardr/wadllib/convert-datetime-309950 into lp:~launchpad-pqm/wadllib/devel

[Leonard Richardson]

[Leonard Richardson (leonardr)]

This is the first wadllib branch in quite a while. It gives wadllib
knowledge about parameters with a type of "xsd:date" or
"xsd:dateTime". It will automatically serialize the values of such
parameters to Python datetime objects.

It would be nice if it could use the current document's XSD namespace
instead of assuming "xsd", but it's not necessary for our purposes and
it seems like a fair amount of work. Let me know what you think.

I'm not sure why a number of these changes (especially the licensing
stuff) aren't already in the repository, but they show up in the diff
against LP trunk, and they're not hard to review.

This branch includes iso_strptime.py, a file contributed by Markus
Korn. We are still waiting for Amanda to come back with rules for
accepting outside contributions, so this branch can't actually land
until we figure out whether, eg. we need Markus to sign a copyright
release.

To test this branch, check out a Launchpad branch, set it up, and then
change its sourcecode/wadllib to point to the wadllib branch. Or you
can run nose on the wadllib branch if you can remember the syntax. I
find it easier to just do it through Launchpad.

I expected this test to cause launchpadlib tests to fail, but I didn't
expect the specific failures I got. Let me know what failures you get
and we'll figure it out.

[lp:~leonardr/wadllib/convert-datetime-309950]

9. By Leonard Richardson on 2009-01-21

Added missing code and became more liberal in accepting dates and datetimes.

[Gary Poster (gary)]

*needs-reply (Edwin)

This seems like a very nice new feature.

I tried to run the tests with nose, as you implied might work. At least with
a simple run, nose fails a few tests because it does not recognize (is not
told that) the ELLIPSIS and NORMALIZE_WHITESPACE flags are supposed to be on.
This would be fixed with making wadllib use zc.buildout and the zope
testrunner, like the lazr packages. Barring objections, I may do that
soonish. For now, I think having the tests run in the Launchpad testrunner is
OK.

Comments below.

> === modified file 'wadllib/application.py'
> --- wadllib/application.py 2008-08-13 13:26:16 +0000
> +++ wadllib/application.py 2009-01-21 19:58:38 +0000
> @@ -43,6 +43,8 @@
> 'WADLError',
> ]
>
> +import datetime
> +import time
> import urllib
> import simplejson
> try:
> @@ -52,6 +54,8 @@
> import cElementTree as ET
> except ImportError:
> import elementtree.ElementTree as ET
> +
> +from iso_strptime import iso_strptime
> from wadllib._utils import uri
>
> def wadl_tag(tag_name):
> @@ -424,7 +428,22 @@
> raise NotImplementedError(
> "Don't know how to find value for a parameter of "
> "type %s." % parameter.style)
> - return self.representation[parameter.name]
> + value = self.representation[parameter.name]
> + if value is not None:
> + if parameter.type in ['xsd:dateTime', 'xsd:date']:

As you said in the cover letter, it would be nice if this were using proper
namespacing. In the "Parsing with Prefixes" section of
http://effbot.org/zone/element-namespaces.htm that doesn't look too bad--at
least he seems to have a recipe for an almost-identical situation.

Perhaps I suggest looking that over and seeing if that can be incorporated
quickly? If not, I'm OK with this for now, particularly if there is an XXX
and a bug number.

> + try:
> + # Parse it as an ISO 1601 date and time.
> + value = iso_strptime(value)
> + except ValueError:
> + # Parse it as an ISO 1601 date.
> + try:
> + value = datetime.datetime(
> + *(time.strptime(value, "%Y-%m-%d")[0:6]))
> + except ValueError:
> + # Oh well. Leave it as a string.
> + pass

Does wadllib have an error channel for this sort of thing? logging seems
minimally appropriate.

Alternatively, I think raising an error would be appropriate, and more
"Pythonic".

A strawman: if the value is None, you leave it alone. If the value parses to
a datetime, you parse it. If it is a string that doesn't parse (or anything
else), you raise an error, with the bad value as an attribute.

If wadl allows you to specify whether a value may be None, then that could be
a better, tighter constraint (that is, if it can be None, let it through;
otherwise raise an error).

That's what I would recommend and prefer.

> +
> + return value
>
> raise NotImplementedError("Pat...

review: Needs Fixing (code)

[Edwin Grubbs (edwin-grubbs)]

Leonard,

I think that Gary makes some very good points, and I would like to know if you disagree with any of the changes he suggests. I only see one other typo. Since this branch does not have a review requested from the ~launchpad team, I'm unable to vote on this merge-proposal.

> === modified file 'wadllib/docs/wadllib.txt'
> --- wadllib/docs/wadllib.txt 2008-08-11 17:56:11 +0000
> +++ wadllib/docs/wadllib.txt 2009-01-21 19:58:38 +0000
> @@ -116,8 +116,8 @@
> test data.
>
> >>> bound_service_root = bind_to_testdata(service_root, 'root')
> - >>> bound_service_root.parameter_names()
> - ['people_collection_link', 'bugs_collection_link']
> + >>> sorted(bound_service_root.parameter_names())
> + ['bugs_collection_link', 'people_collection_link']
> >>> [method.id for method in bound_service_root.method_iter]
> ['service-root-get']
>
> @@ -266,7 +266,8 @@
>
> >>> bound_limi = bind_to_testdata(limi_person, 'person-limi')
> >>> sorted(bound_limi.parameter_names())[:3]
> - ['admins_collection_link', 'confirmed_email_addresses_collection_link', 'date_created']
> + ['admins_collection_link', 'confirmed_email_addresses_collection_link',
> + 'date_created']
> >>> languages_link = bound_limi.get_parameter("languages_collection_link")
> >>> languages_link.get_value()
> u'http://api.launchpad.dev/beta/~limi/languages'
> @@ -302,7 +303,7 @@
> an example.
>
> There's a method on a person resource such as bound_limi that's
> -identified by a distinctive query argument: ws.op=findPathToTeam.
> +identified by a distinctive query argument: ws.op=getMembersByStatus.
>
> >>> method = bound_limi.get_method(
> ... query_params={'ws.op' : 'findPathToTeam'})
> @@ -353,6 +354,51 @@
> >>> print method.response.get_representation_definition('text/html')
> None
>
> +=== Data type converstion ===

s/converstion/conversion/

[lp:~leonardr/wadllib/convert-datetime-309950]

10. By Leonard Richardson on 2009-01-22

Added namespace support when extracting a parameter's type.

11. By Leonard Richardson on 2009-01-22

Changed error handling when a date parameter has a value that can't be converted into a date.

12. By Leonard Richardson on 2009-01-22

Corrected line length.

[Leonard Richardson (leonardr)]

OK, this should address everyone's comments.

=== modified file 'wadllib/application.py'
--- wadllib/application.py 2009-01-21 19:12:41 +0000
+++ wadllib/application.py 2009-01-22 15:33:42 +0000
@@ -43,6 +43,7 @@
     'WADLError',
     ]

+from cStringIO import StringIO
 import datetime
 import time
 import urllib
@@ -58,6 +59,9 @@
 from iso_strptime import iso_strptime
 from wadllib._utils import uri

+NS_MAP = "xmlns:map"
+XML_SCHEMA_NS_URI = 'http://www.w3.org/2001/XMLSchema'
+
 def wadl_tag(tag_name):
     """Scope a tag name with the WADL namespace."""
     return '{http://research.sun.com/wadl/2006/10}' + tag_name
@@ -411,7 +415,11 @@
         return None

     def get_parameter_value(self, parameter):
- """Find the value of a parameter, given the Parameter object."""
+ """Find the value of a parameter, given the Parameter object.
+
+ :raise ValueError: If the parameter value can't be converted into
+ its defined type.
+ """

         if self.representation is None:
             raise NoBoundRepresentationError(
@@ -430,25 +438,46 @@
                     "type %s." % parameter.style)
             value = self.representation[parameter.name]
             if value is not None:
- if parameter.type in ['xsd:dateTime', 'xsd:date']:
+ namespace_url, data_type = self._dereference_namespace(
+ parameter.tag, parameter.type)
+ if (namespace_url == XML_SCHEMA_NS_URI
+ and data_type in ['dateTime', 'date']):
                     try:
- # Parse it as an ISO 1601 date and time.
+ # Parse it as an ISO 8601 date and time.
                         value = iso_strptime(value)
                     except ValueError:
- # Parse it as an ISO 1601 date.
+ # Parse it as an ISO 8601 date.
                         try:
                             value = datetime.datetime(
                                 *(time.strptime(value, "%Y-%m-%d")[0:6]))
                         except ValueError:
- # Oh well. Leave it as a string.
- pass
-
+ # Raise an unadored ValueError so the client
+ # can treat the value as a string if they
+ # want.
+ raise ValueError(value)
             return value

         raise NotImplementedError("Path traversal not implemented for "
                                   "a representation of media type %s."
                                   % self.media_type)

+
+ def _dereference_namespace(self, tag, value):
+ """Splits a value into namespace URI and value.
+
+ :param tag: A tag to use as context when mapping namespace
+ names to URIs.
+ """
+ namespace_url = None
+ if value is not None and ':' in value:
+ namespace, value = value.split(':', 1)
+ else:
+ namespace = ''
+ if namespace is not None:
+ ns_map = tag.get(NS_MAP)
+ namespace_url = ns_map.get(namespace)
+ ...

[Gary Poster (gary)]

*merge-conditional (Edwin)

Looks good to me. Thank you.

I saw two spelling errors and I suggest we rearrange the regex comments.

In application.py:

> + # Raise an unadored ValueError so the client
> + # can treat the value as a string if they
> + # want.

s/unadored/unadorned/

and in iso_strptime.py:

> + (?P<year>\d{4})\-(?P<month>\d{2})\-(?P<day>\d{2})
> + # pattern matching date
> + T
> + # seperator
> + (?P<hour>\d{2})\:(?P<minutes>\d{2})\:(?P<seconds>\d{2})
> + # pattern matching time
> + (\.(?P<microseconds>\d{6}))?
> + # pattern matching optional microseconds
> + (?P<tz_offset>[\-\+]\d{2}\:\d{2})?
> + # pattern matching optional timezone offset

s/seperator/separator/

Also I think the comments should now go before the pertinent part of the pattern:

+ # pattern matching date
+ (?P<year>\d{4})\-(?P<month>\d{2})\-(?P<day>\d{2})
+ # separator
+ T
+ # pattern matching time
+ (?P<hour>\d{2})\:(?P<minutes>\d{2})\:(?P<seconds>\d{2})
+ # pattern matching optional microseconds
+ (\.(?P<microseconds>\d{6}))?
+ # pattern matching optional timezone offset
+ (?P<tz_offset>[\-\+]\d{2}\:\d{2})?

Gary

review: Approve (code)

[Edwin Grubbs (edwin-grubbs)]

merge-conditional

Hi Leonard,

I just have one item to add.

=== modified file 'wadllib/application.py'
--- wadllib/application.py>-----2009-01-21 19:12:41 +0000
+++ wadllib/application.py>-----2009-01-22 21:29:11 +0000
+ def _dereference_namespace(self, tag, value):
+ """Splits a value into namespace URI and value.
+
+ :param tag: A tag to use as context when mapping namespace
+ names to URIs.
+ """
+ namespace_url = None
+ if value is not None and ':' in value:
+ namespace, value = value.split(':', 1)
+ else:
+ namespace = ''
+ if namespace is not None:

This if-statement isn't necessary, since value.split()
will always set it to a string. It's also easy to miss the
default value for namespace_url above, which isn't necessary
if you remove the if-statement.

+ ns_map = tag.get(NS_MAP)
+ namespace_url = ns_map.get(namespace)
+ return namespace_url, value
+

[Edwin Grubbs (edwin-grubbs)]

review: Approve

[lp:~leonardr/wadllib/convert-datetime-309950]

13. By Leonard Richardson on 2009-01-22

Removed __main__ section.

14. By Leonard Richardson on 2009-01-23

Response to feedback.

[Leonard Richardson (leonardr)]

Ok, all this stuff is fixed. Now we're just waiting on copyright assignment rules.

[lp:~leonardr/wadllib/convert-datetime-309950]

15. By Leonard Richardson on 2009-01-23

Fixed spelling mistakes.