Code review comment for lp:~frankban/juju-quickstart/env-manage-base-views

*** Submitted:

Quickstart base structure for views.

This branch implements a way to start
Urwid interactive sessions in quickstart.
This infrastructure will be used in
future branches to implement the
quickstart environment management system.

My apologies for the long diff, but you
can safely ignore all the file headers,
and the code includes long module
docstrings which can help reviewing the
branch.

Tests: `make check`.

QA:
run `make` and then `./cli-app-demo.py`:
this will start a demo application using
the views infrastructure.

Thank you!

R=gary.poster, bac
CC=
https://codereview.appspot.com/42600044

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

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/base.py#newcode19
quickstart/cli/base.py:19: A collection of objects which help building a
Quickstart CLI application
On 2013/12/17 14:30:47, bac wrote:
> change 'building' -> 'in building' or 'build'

Done.

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/base.py#newcode55
quickstart/cli/base.py:55: Improve the level of event loop introspection
so that code and tests
On 2013/12/17 14:30:47, bac wrote:
> Improves

Done.

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/base.py#newcode111
quickstart/cli/base.py:111: def set_contents(listbox):
On 2013/12/17 14:30:47, bac wrote:
> In the docstring you call the param a generic 'widget'. They should
match. If
> not generic then should you validate that the param is a listbox? I
think using
> a generic widget is better.

Done.

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/base.py#newcode112
quickstart/cli/base.py:112: contents.original_widget = listbox
On 2013/12/17 14:30:47, bac wrote:
> original_widget seems like an odd name since it is really just the
current
> widget.

Unfortunately this is how the wrapped widget is named in Urwid.

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

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/ui.py#newcode28
quickstart/cli/ui.py:28: # Define the color palette used by the Urwid
application.
On 2013/12/17 14:00:43, gary.poster wrote:
> Might be nice to describe the columns. I assume they are class name
(or None
> for default), text style, background style.

Done.

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/ui.py#newcode70
quickstart/cli/ui.py:70: def bind(function, *args):
On 2013/12/17 14:00:43, gary.poster wrote:
> Huh. So...this effectively produces a thunk that discards/ignores the
argument.
> You could call it that, I guess. This functionality is really
generic. If you
> didn't need to discard the argument, you could just use
functools.partial.

> def make_aggressive_thunk(function, *args, **kwargs):
> def thunk(*ignored, **ignored_kwargs):
> return function(*args, **kwargs)
> return thunk

> <shrug> :-)

Renamed to thunk.

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

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/views.py#newcode51
quickstart/cli/views.py:51: pressing a keyboard shortcut;
On 2013/12/17 14:30:47, bac wrote:
> by pressing

Done.

https://codereview.appspot.com/42600044/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/42600044/diff/1/quickstart/utils.py#newcode194
quickstart/utils.py:194:
http://www.tornadoweb.org/en/stable/_modules/tornado/util.html#ObjectDict
On 2013/12/17 14:30:47, bac wrote:
> Does the use here complicate our copyright statement above? You've
given
> attribution but it looks like Canonical is claiming copyright to it.

> Perhaps we include at the top the copyright statement from tornado
with the
> reference to ObjectDict.

> # The ObjectDict code:
> #
> # Copyright 2009 Facebook
> #
> # Licensed under the Apache License, Version 2.0 (the "License"); you
may
> # not use this file except in compliance with the License. You may
obtain
> # a copy of the License at
> #
> # http://www.apache.org/licenses/LICENSE-2.0
> #
> # Unless required by applicable law or agreed to in writing, software
> # distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT
> # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See
the
> # License for the specific language governing permissions and
limitations
> # under the License.

Very good point, done, thank you.

https://codereview.appspot.com/42600044/