yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Hi Thiago.  Thank you for the quick turnaround on this fix.  I have only
done a code review of this so far, and not a functional test/review.
However, I have some concerns with the way you changed the tests, and
I'd like to see those resolved before I approve.

Gary


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

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#oldcode96
app/modules-debug.js:96: requires: ['juju-charm-id'],
I don't really understand why these were here instead of in the modules
to begin with.  That makes me nervous, because often people have a good
reason for this sort of thing.  Do you know the answer?

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

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode4
app/modules-debug.js:4: // property) and the "fullpath" of the file that
implement a given module. The
Might as well take the opportunity to be clearer about what the "use"
property does.  ("""This file declares which files implement modules,
using the "fullpath" property; and declares the membership of rollup
modules, using the "use" property to specify what the module name
aliases.""" or something like that.)

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode5
app/modules-debug.js:5: // "requires" property should not be used here.
Please explain why  ("...should not be used here because the javascript
minimizer will not parse it" or something like that).

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode16
app/modules-debug.js:16: modules: {
I really am tempted to suggest that the production files should parse
this and rewrite it after removing the "fullpath" modules, instead of
making developers maintain two versions of this "modules" file.  Even if
that's a good idea, that's a separate bug/branch.  I'm just thinking.

https://codereview.appspot.com/6856070/diff/3001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/6856070/diff/3001/test/index.html#newcode40
test/index.html:40: YUI().use('node', 'event', function(runnerY) {
In terms of scoping rules, I'm pretty sure that this could be "Y" and
the one used by tests, below, could be "Y" also.  Did you not do it that
way because you felt it was more readable this way?

https://codereview.appspot.com/6856070/diff/3001/test/index.html#newcode61
test/index.html:61: window.Y = testsYUI;
We usually create our own Y in the tests.  Why do we have to do this?
Wouldn't it be better to make our own Y in each test?

https://codereview.appspot.com/6856070/diff/3001/test/test_app.js
File test/test_app.js (left):

https://codereview.appspot.com/6856070/diff/3001/test/test_app.js#oldcode184
test/test_app.js:184: 'json-stringify',
This was specifying different requirements than you have put in the
global Y.  We really should not share the Y object.  We should keep our
dependencies tight to our tests in order to increase the maintainability
and understandability of our code.  This comment addresses all changes
like this one.

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.