yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

Thanks for your review, guys!



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'],
On 2012/11/20 21:01:19, gary.poster wrote:
> 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?

It is not their fault. The problem is that YUI has two places to define
the very same thing... which is not bad when we are loading non yui js
files that depend on other js files.

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
On 2012/11/20 21:01:19, gary.poster wrote:
> 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.)

Done.

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode5
app/modules-debug.js:5: // "requires" property should not be used here.
On 2012/11/20 21:01:19, gary.poster wrote:
> Please explain why  ("...should not be used here because the
javascript
> minimizer will not parse it" or something like that).

Done.

https://codereview.appspot.com/6856070/diff/3001/app/modules-debug.js#newcode16
app/modules-debug.js:16: modules: {
On 2012/11/20 21:01:19, gary.poster wrote:
> 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.

I am tempted to simply remove the aliases. They are one of the reasons
we cannot completely turn the loader off, remember?

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) {
On 2012/11/20 21:01:19, gary.poster wrote:
> 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?

Reverting...

https://codereview.appspot.com/6856070/diff/3001/test/index.html#newcode61
test/index.html:61: window.Y = testsYUI;
On 2012/11/20 21:01:19, gary.poster wrote:
> 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?

Reverting...

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',
On 2012/11/20 21:01:19, gary.poster wrote:
> 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.

Done.

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.