Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(124)

Issue 71230043: Constraint parsing from charmworldlib.

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years ago by bac
Modified:
10 years ago
Reviewers:
mp+209327, frankban
Visibility:
Public.

Description

Constraint parsing from charmworldlib. The constraint parsing code has been moved to charmworldlib so it has been deleted here. Add dependency on charmworldlib and include it in deps. https://code.launchpad.net/~bac/charms/precise/juju-gui/constraint-parsing-deux/+merge/209327 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8

Patch Set 2 : Constraint parsing from charmworldlib. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -103 lines) Patch
M HACKING.md View 1 1 chunk +4 lines, -0 lines 0 comments Download
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
A deps/charmworldlib-0.3.0.tar.gz View 0 chunks +-1 lines, --1 lines 0 comments Download
M server-requirements.pip View 1 chunk +1 line, -0 lines 0 comments Download
M server/guiserver/bundles/utils.py View 1 5 chunks +3 lines, -41 lines 0 comments Download
M server/guiserver/tests/bundles/test_utils.py View 1 chunk +22 lines, -62 lines 0 comments Download
M tests/20-functional.test View 2 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 3
bac
Please take a look.
10 years ago (2014-03-04 18:46:32 UTC) #1
frankban
Nice branch Brad, thank you! LGTM with minor changes. https://codereview.appspot.com/71230043/diff/1/HACKING.md File HACKING.md (right): https://codereview.appspot.com/71230043/diff/1/HACKING.md#newcode72 HACKING.md:72: ...
10 years ago (2014-03-05 13:07:54 UTC) #2
bac
10 years ago (2014-03-05 15:23:24 UTC) #3
*** Submitted:

Constraint parsing from charmworldlib.

The constraint parsing code has been moved to charmworldlib so it has been
deleted here.

Add dependency on charmworldlib and include it in deps.

R=frankban
CC=
https://codereview.appspot.com/71230043

https://codereview.appspot.com/71230043/diff/1/HACKING.md
File HACKING.md (right):

https://codereview.appspot.com/71230043/diff/1/HACKING.md#newcode72
HACKING.md:72: Note that the test will not work using an LXC environment.  The
test
On 2014/03/05 13:07:54, frankban wrote:
> Thank you for this! To be more explicit, we could say "Note that functional
> tests will not work...".

Done.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (left):

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:40: # Define a sequence of allowed constraints
to be used in the process of
On 2014/03/05 13:07:54, frankban wrote:
> I am very happy that now we have a shared lib at least for this constraints
> stuff.

Done.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (right):

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:34: from charmworldlib.utils import
parse_constraints
On 2014/03/05 13:07:54, frankban wrote:
> Please keep those in alphabetical order.

Done.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:160: - the bundle includes unsupported
constraints.
On 2014/03/05 13:07:54, frankban wrote:
> I see that parse_constraints raises a ValueError if constraints are not valid.
> Maybe it's worth mentioning here too, or below, e.g.
> - the bundle includes unsupported or invalid constraints.
> 
> We also might want to mention somewhere (perhaps in the requirements file)
that
> when updating the charmworldlib tgz, we should ensure parse_constraints still
> only raises ValueErrors.

I made the first change.  The reminder seems like a good idea but would not
scale.  I mean we have lots of dependencies so making notes about double
checking exceptions seems unworkable.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b