Closed vitkarpov closed 9 years ago
Nice! This looks good. I've added an inline comment about one small nit.
Could you also write tests for this? Thanks!
How to write tests? I think it should be some util getExtraScriptsAndStyles.coffee
with some interface, for instance, it gets files' list and returns promise which should be resolved with files' contents and spec for that module.
Than I can just use it in generator.coffee
. Am I right?
That sounds good. So, you move the function to load external files to a separate module/file. You can use that module in generator.coffee
.
To test that, you first create some example files that you want to include (e.g. in /test/examples
). Then, write a test (e.g. /test/util/include_external_files.coffee
) that checks that your new module, called with the correct arguments, returns a promise that resolves with an object that contains your example files.
Finally, you may also add some more tests for different cases, e.g. one that ensures that you throw a useful error when yo try to load a file that doesn't exist.
@killercup Hi, happy new year!
After a long break it seems I've finished here.
I created a separate helper getExtraContent
. It demands an array of paths and returns promise which should be resolved with contents of that files. The main loop in generator
now looks like: read contents of extra files, then do another bunch of stuff that has been before, asynchronously.
I made some tests for the helper and updated README and CLI help according to the new options.
Thanks! A happy new year to you to! What you describe sounds really promising. I'll have a look at it later today.
On Fri, Jan 9, 2015 at 2:58 PM, Viktor Karpov notifications@github.com wrote:
@killercup Hi, happy new year! After a long break it seems I've finished here. I created a separate helper
getExtraContent
. It demands array of paths and returns promise which should be resolved array and tests for it. The main loop ingenerator
now looks like: read contents of extra files, then do another bunch of stuff that has been before, asynchronously.Also, I updated README and CLI help according to the new options.
Reply to this email directly or view it on GitHub: https://github.com/killercup/grock/pull/24#issuecomment-69336873
Thanks, @vitkarpov, this looks really good!
I left a few comment in the inline diff of the generator function. Basically, I think the promise handling could be optimized a bit. Let me outline some code:
getExtraContent()
.then (scripts) ->
# ...
deferred = Q.defer()
vfs.src(stuff).pipe(stuff)
.on (end) ->
style.copy(dest: dest).then(deferred.resolve, deferred.reject)
.on (err) -> deferred.reject(err)
return deferred.promise
.then(
(res) -> log "Done.", colors.magenta("Generated in"), duration(start)
(err) -> log colors.red err
)
Basically, I would like to have the logging in one then statement below and move the deferred into the first promise handling.
Let me know what you think. If I have time this weekend, I'll have another look at this and maybe merge this with a few changes of my own.
Thanks again! Have a great weekend!
Let me know what you think. If I have time this weekend, I'll have another look at this and maybe merge this with a few changes of my own.
Feel free to make some changes, I think the idea is clear. If you wanna me make some changes too, just let me know.
@vitkarpov Merged! I've made some changes (removed async, whitespace stuff) in a few commits. You can see my recent updates here: https://github.com/killercup/grock/compare/fc4c489b46c02eb516637a0c5e4e5afe61ea4d7e...5ef2f4f760cf53c8e4bdedca17fdcfc037f97c13?w=1
@vitkarpov Could you perhaps test the latest version on master a bit? Just to see if everything works. I only tested it with grock's own code. I'll release a new version when I know it works for you. :)
Cool, thanks! :+1:
@killercup I've checked on my project — works fine!
@vitkarpov Just published 0.3.7: https://www.npmjs.com/package/grock
Thanks again for your contribution!
Hi!
I've added options
ext-scripts
andext-styles
: arrays of paths for externals styles and scripts. Contents of the files append to the corresponding sections in templates.Check out this way.
TODO: