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

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

LGTM and QA-OK. Some changes suggested below but nothing substantial.
Thanks for the interesting work.

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
change 'building' -> 'in building' or 'build'

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
Improves

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/base.py#newcode111
quickstart/cli/base.py:111: def set_contents(listbox):
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.

https://codereview.appspot.com/42600044/diff/1/quickstart/cli/base.py#newcode112
quickstart/cli/base.py:112: contents.original_widget = listbox
original_widget seems like an odd name since it is really just the
current widget.

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;
by pressing

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
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.

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

« Back to merge proposal