yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

Tkx Benji.


https://codereview.appspot.com/6821090/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode12
Makefile:12: node_modules/grunt node_modules/node-spritesheet
node_modules/node-minify
On 2012/11/07 22:00:47, benji wrote:
> On 2012/11/07 18:59:19, thiago wrote:
> > This line does not have more than 80 chars, but I changed it anyway.

> Correct, it has fewer than 80 characters but one of those characters
is a tab,
> which in most editors displays as 8 spaces, pushing the line over 80
displayed
> characters.



Ok

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js
File app/assets/combine/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode1
app/assets/combine/merge-files.js:1: 'use strict';
On 2012/11/07 22:00:47, benji wrote:
> On 2012/11/07 18:59:19, thiago wrote:
> > This is entirely ours.

> Top-posting makes replying to comments difficult.  Please reply below
the
> previous comment.

Ok

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode3
app/assets/combine/merge-files.js:3: //
http://stackoverflow.com/questions/5348685/node-js-require-inheritance
On 2012/11/07 22:00:47, benji wrote:
> Even after reading the page at the other end of this link I don't know
what it
> has to do with the code here.  Additional comments would help.

Done.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode5
app/assets/combine/merge-files.js:5:
On 2012/11/07 22:00:47, benji wrote:
> I am surprised that we had to write this script.  I would have
expected someone,
> somewhere to have already done this for us.

Not really. People usually use the comboloader for yui applications. I
had a lot of fun writing it though.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode14
app/assets/combine/merge-files.js:14: var execution = new
compressor.minify({
On 2012/11/07 22:00:47, benji wrote:
> object literal style

How should it be?

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode88
app/assets/combine/merge-files.js:88: var reqs = (function() {
On 2012/11/07 22:00:47, benji wrote:
> This function would benefit from a name.

This is not a function. This is an object: a list of all the yui
requirements we need to load.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode105
app/assets/combine/merge-files.js:105: // Using the example
http://yuilibrary.com/yui/docs/yui/loader-resolve.html
On 2012/11/07 22:00:47, benji wrote:
> I am not sure what this comment is trying to communicate.  Is it that
I can find
> an explanation of the YUI loader mechanism at the given URL?

Nop. It uses the reqs object that we created in the previous block and
figures out the js files that contain it.

I will add it as comment here.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode111
app/assets/combine/merge-files.js:111: loader = new Y.Loader({
On 2012/11/07 22:00:47, benji wrote:
> object literal style

How should it be?

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode126
app/assets/combine/merge-files.js:126: (function() {
On 2012/11/07 22:00:47, benji wrote:
> Why are there all of these anonymous functions that are just called
immediately?

The have different scopes. So I can separate concerns and I can reuse
variables names :O)

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode142
app/assets/combine/merge-files.js:142: outputFile =
syspath.join(process.cwd(),
On 2012/11/07 22:00:47, benji wrote:
> This looks redundant.  Why do we need to join the current working
directory with
> a path that would have been interpreted as relative to the CWD anyway?

I removed some of them, but I still need to keep two calls to it. You
will see it in the new proposal.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode152
app/assets/combine/merge-files.js:152: (function() {
On 2012/11/07 22:00:47, benji wrote:
> This and the above two function -- and, to a lesser extent, the one
above that
> -- have very similar structures.  Here is a refactoring that combines
all four
> into a smaller function: http://paste.ubuntu.com/1341063/

Nice. Done.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode162
app/assets/combine/merge-files.js:162: })();
On 2012/11/07 22:00:47, benji wrote:
> This program is sufficiently large that it would benefit from tests.
They would
> have helped me a great deal while doing my refactoring.

This is not part of the application. This is a build tool. Do we need to
create tests for it?

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode163
app/assets/combine/merge-files.js:163:
On 2012/11/07 22:00:47, benji wrote:
> There is an unnecessary blank line at the end of the file.

It seems all our files have it.

https://codereview.appspot.com/6821090/

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