Merge lp:~wallyworld/lazr.restful/dict-parameters into lp:lazr.restful
Proposed by
Ian Booth
Status: | Merged |
---|---|
Merged at revision: | 199 |
Proposed branch: | lp:~wallyworld/lazr.restful/dict-parameters |
Merge into: | lp:lazr.restful |
Diff against target: |
351 lines (+180/-12) 6 files modified
.bzrignore (+1/-0) src/lazr/restful/NEWS.txt (+6/-0) src/lazr/restful/configure.zcml (+7/-0) src/lazr/restful/docs/webservice-marshallers.txt (+83/-6) src/lazr/restful/marshallers.py (+82/-5) src/lazr/restful/version.txt (+1/-1) |
To merge this branch: | bzr merge lp:~wallyworld/lazr.restful/dict-parameters |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Francis J. Lacoste (community) | Approve | ||
Benji York | Pending | ||
Review via email: mp+97128@code.launchpad.net |
Commit message
Add support for marshalling exported method parameters of type dict.
Description of the change
== Implementation ==
Add the zcml registration and implementation class for a dictionary marshaller: DictFieldMarsha
Also fixed some drive-by lint issues.
== Tests ==
Add new tests to the webservice-
== Lint ==
Linting changed files:
src/lazr/
src/lazr/
src/lazr/
src/lazr/
src/lazr/
./src/lazr/
157: E261 at least two spaces before inline comment
To post a comment you must log in.
Hi Ian,
Thank you very much for this addition to lazr.restful!
I'm approving this on the premise that you'll address my main comment.
[1]
255 + def __init__(self, field, request): shaller` . Marshaller, self).__init__( marshaller = getMultiAdapter(
256 + """See `SimpleFieldMar
257 +
258 + This also looks for the appropriate marshaller for key_type and
259 + value_type.
260 + """
261 + super(DictField
262 + field, request)
263 + self.key_marshaller = getMultiAdapter(
264 + (field.key_type, request), IFieldMarshaller)
265 + self.value_
266j+ (field.value_type, request), IFieldMarshaller)
I think we should probably raise an error if key_type or value_type aren't defined?
[2]
307 + @property collection_ factory( self):
308 + def _python_
309 + """Create the appropriate python collection from a dict."""
310 + # For the DictMarshaller, _type contains the type object,
311 + # which can be used as a factory.
312 + return self.field._type
Why this indirection? Do you really expect users to subclass the marshaller to
override the default? YAGNI
[3]
300 + value = super( ller, self)._ marshall_ from_json_ data(value)
301 + DictFieldMarsha
Why is this needed?
value is already a dict, what do you expect the superclass to handle? Ah,
I think it's error handling? Maybe add a comment about that? Which actually
points to the fact that you are missing a test for this. You test that ValueError
are raised if the key or value are of the improper type, but not if the request/json is
not actually a dict (or of the proper representation).
Thanks for the drive-by formatting fixes.
Cheers