search

yeoman / doctor

Detect potential issues with users system that could prevent Yeoman from working correctly
http://yeoman.io
BSD 2-Clause "Simplified" License
61 stars 17 forks source link

Async & Message templates #13

Closed j3lte closed 9 years ago

j3lte commented 9 years ago

See what you can use....

One thing to notice, I have switched off two tests, because they fail (need to be rewritten). Now, ofcourse that is not the way to do it ;-). They fail because the message templates are loaded through fs. These fs readFileSync requests are redefined in the test (And so, the expected outcome is not the same, allthough it functions properly)

SBoudrias commented 9 years ago

Also, can you give every template file an extension for twig so we get syntax highlight in the editors.

Otherwise this looks mostly good. Thanks for the hard work.

Next time, it is better to keep every feature separated in it's own PR. Merging them together makes it hard to review as there is too many concerns in a single PR. This should've been two PR: async flow and templating.

j3lte commented 9 years ago

Thanks for the extensive comments (very helpful), will create the changes tomorrow. And yes, it was stupid to put it all in one PR. I should have created different branches for that, right? Will see what I can do :-)

SBoudrias commented 9 years ago

I already went over and review, don't worry about splitting it in two now. I was just saying for the future :)

j3lte commented 9 years ago

Every comment is accounted for :smile: Tested & works

SBoudrias commented 9 years ago

Ah this looks very good. It'd be great if you can fix the test too, otherwise I'll probably come around it this weekend.

j3lte commented 9 years ago

I have probably overlooked this bowerrc !== yorc when copy-pasting. Fixed in the commit.

The test, I do not have enough knowledge to rewrite that one. Will look into it, otherwise I would very much like to see how you fix it this weekend :smile:

j3lte commented 9 years ago

See last commit. Test is fixed now. Had to load the message before, and make the stub fs readFileSync respond with the file.

SBoudrias commented 9 years ago

Ok, that's merged into master! Thanks a ton!