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/