Merge lp://qastaging/~frankban/juju-quickstart/views-params into lp://qastaging/juju-quickstart

Proposed by Francesco Banconi
Status: Merged
Merged at revision: 111
Proposed branch: lp://qastaging/~frankban/juju-quickstart/views-params
Merge into: lp://qastaging/juju-quickstart
Diff against target: 975 lines (+301/-172)
6 files modified
quickstart/cli/params.py (+51/-0)
quickstart/cli/views.py (+70/-74)
quickstart/manage.py (+11/-4)
quickstart/tests/cli/test_params.py (+87/-0)
quickstart/tests/cli/test_views.py (+71/-90)
quickstart/tests/test_manage.py (+11/-4)
To merge this branch: bzr merge lp://qastaging/~frankban/juju-quickstart/views-params
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+245753@code.qastaging.launchpad.net

Description of the change

Refactor view parameters.

Implement a Params object used to store
common view parameters. This way it will
be easier to add parameters in the future
(e.g. one callable to remove stale jenv files).

New code include the params module and a fix
to the code handling the listing of jenv files
in the index view: now the header message is
only displayed if jenv files actually exist.

The rest of the diff is mechanical: i.e. replacing
the single view arguments with the params named
tuple.

Tests: `make check`.

QA:
Use the quickstart interactive session and
check everything works ok.

https://codereview.appspot.com/194030043/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :

Reviewers: mp+245753_code.launchpad.net,

Message:
Please take a look.

Description:
Refactor view parameters.

Implement a Params object used to store
common view parameters. This way it will
be easier to add parameters in the future
(e.g. one callable to remove stale jenv files).

New code include the params module and a fix
to the code handling the listing of jenv files
in the index view: now the header message is
only displayed if jenv files actually exist.

The rest of the diff is mechanical: i.e. replacing
the single view arguments with the params named
tuple.

Tests: `make check`.

QA:
Use the quickstart interactive session and
check everything works ok.

https://code.launchpad.net/~frankban/juju-quickstart/views-params/+merge/245753

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/194030043/

Affected files (+303, -172 lines):
   A [revision details]
   A quickstart/cli/params.py
   M quickstart/cli/views.py
   M quickstart/manage.py
   A quickstart/tests/cli/test_params.py
   M quickstart/tests/cli/test_views.py
   M quickstart/tests/test_manage.py

Revision history for this message
Brad Crittenden (bac) wrote :

LGTM. No QA yet.

https://codereview.appspot.com/194030043/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/194030043/diff/1/quickstart/cli/views.py#newcode314
quickstart/cli/views.py:314: # and supposed to be working/active. The
user has the ability to select
Maybe 'assumed' would be clearer than 'supposed'?

https://codereview.appspot.com/194030043/diff/1/quickstart/tests/cli/test_params.py
File quickstart/tests/cli/test_params.py (right):

https://codereview.appspot.com/194030043/diff/1/quickstart/tests/cli/test_params.py#newcode87
quickstart/tests/cli/test_params.py:87: self.assertNotEqual(self.params,
params)
Such a nice set of tests! Thanks for the clarity.

https://codereview.appspot.com/194030043/

Revision history for this message
Martin Hilton (martin-hilton) wrote :
Revision history for this message
Brad Crittenden (bac) wrote :

QA-OK. I simply ran quickstart and launched a local provider.
Everything came up as expected.

https://codereview.appspot.com/194030043/

114. By Francesco Banconi

Improve view comment.

Revision history for this message
Francesco Banconi (frankban) wrote :

*** Submitted:

Refactor view parameters.

Implement a Params object used to store
common view parameters. This way it will
be easier to add parameters in the future
(e.g. one callable to remove stale jenv files).

New code include the params module and a fix
to the code handling the listing of jenv files
in the index view: now the header message is
only displayed if jenv files actually exist.

The rest of the diff is mechanical: i.e. replacing
the single view arguments with the params named
tuple.

Tests: `make check`.

QA:
Use the quickstart interactive session and
check everything works ok.

R=bac, martin.hilton
CC=
https://codereview.appspot.com/194030043

https://codereview.appspot.com/194030043/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/194030043/diff/1/quickstart/cli/views.py#newcode314
quickstart/cli/views.py:314: # and supposed to be working/active. The
user has the ability to select
On 2015/01/07 15:58:26, bac wrote:
> Maybe 'assumed' would be clearer than 'supposed'?

Done.

https://codereview.appspot.com/194030043/

Revision history for this message
Francesco Banconi (frankban) wrote :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches