Code review comment for lp:~rharding/charmworld/move-bundle-url

Revision history for this message
Benji York (benji) wrote :

LGTM

https://codereview.appspot.com/21370044/diff/20001/charmworld/routes.py
File charmworld/routes.py (right):

https://codereview.appspot.com/21370044/diff/20001/charmworld/routes.py#newcode57
charmworld/routes.py:57:
"/bundle/~{owner}/{basket}/{rev}/{bundle}/json")
On 2013/11/04 21:46:34, rharding wrote:
> this could probably go into some sort of /bundle/*bits but leaving as
simple
> case of moving the url for the moment.

If I'm understanding you correctly, I think I like it better this way.
It is easier to see the pattern matching spelled out in routes rather
than in code.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/api/__init__.py
File charmworld/views/api/__init__.py (right):

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/api/__init__.py#newcode429
charmworld/views/api/__init__.py:429: 'personal-bundle-json-revision',
**route_info)
Since route_info isn't used subsequently, I don't see why it is created
instead of just passing its contents as keyword arguments.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py
File charmworld/views/tests/test_api.py (right):

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py#newcode1
charmworld/views/tests/test_api.py:1: # Copyright<enter change
description here if sensible> 2012, 2013 Canonical
Something bad happened here.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py#newcode841
charmworld/views/tests/test_api.py:841:
u'http://example.com/bundle/%7Ebac/bat/4/byobu/json',
On 2013/11/04 21:46:34, rharding wrote:
> I'm so sorry, but the linter...he hates me.

Eww. That is bad.

It wouldn't accept the value being indented four more spaces?

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py#newcode1585
charmworld/views/tests/test_api.py:1585: class TestAPI3(TestAPI,
API3Mixin):
On 2013/11/04 21:46:34, rharding wrote:
> remove the stupid Z. Just skipping the bad test for now. See later
files.

I like the explicitness of the new approach better.

https://codereview.appspot.com/21370044/

« Back to merge proposal