yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Hi Thiago.  I like what you did with the tests, even though I wish it
didn't have to be part of this branch.  I'm OK with it landing though.

As I mentioned on IRC, I do see a test failure, but it seems shallow.
Please fix that, and maybe the small comment change I suggest.

Thank you,

Gary


https://codereview.appspot.com/6856070/diff/9001/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/6856070/diff/9001/app/modules-debug.js#newcode10
app/modules-debug.js:10: // minimizer will not parse it.
Maybe add "Please put in in the module itself, instead."

https://codereview.appspot.com/6856070/diff/9001/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/6856070/diff/9001/test/test_app.js#newcode34
test/test_app.js:34: YUI(GlobalConfig).use(['juju-gui',
'juju-tests-utils'], function(Y) {
This looks good to me.  However, it is a big change to how we write our
tests, and not consistently applied to our tests.  As I said on IRC, I
think there is a good argument to change our tests to be written this
way, but I don't understand why we need to change them here, in this
branch.

In the interest of progress, I'm ok with landing this if you then make a
retrospective discussion card for us to collectively determine the way
we want to move forward.

https://codereview.appspot.com/6856070/

-- 
https://code.launchpad.net/~tveronezi/juju-gui/change-requires-param/+merge/135198
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~tveronezi/juju-gui/change-requires-param into lp:juju-gui.