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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
