Closed soupman99 closed 8 years ago
@soupman99 Thank you for helping out here.
To make it easier to review, can you provide a summary of these changes? Have you tested this?
@codygaig How is this related to the "Remove Templates" PR?
@mleanos it's pretty much a more complete build out of #131 using the articles module as a template. Essentially it's all the components you'd need to create a new full module. I haven't written any karma tests, but I have been testing as I went along. Everything seems to be working as expected.
@soupman99 Can you provide screen shots? I'll try to pull this down locally and test, over the weekend.
Don't change permissions please for files. Also, we require tests to merge, and that will also allow us to test it constantly against other features we will add, so please add tests.
@soupman99 Thanks for contributing! Few things that need to be cleaned up: don't use the child_process.exec command to manage files. Use the internal fs
module, we also need tests to ensure that it was generated as expected.
@mleanos I'm not sure I understand what you mean by screenshots? You mean of the html templates?
@ilanbiala I didn't realize I had changed file permissions. I'm new to github. Could you be a little more specific?
@soupman99 I meant screen shots of you using the generator to create a new module. It's one way for us to see it in action, other than pulling this branch down locally. I plan to test myself, but it would be nice for others to be able to see it.
@soupman99 we've merged in the remove templates PR, and we'd like to include this or #131 in the next release very soon. Can you double check to see if anything in this PR needs to be changed. @codydaig functionally does it LGTY?
@ilanbiala I left some more line comments that need too get addressed. The commits also need too be squashed. I haven't tested the code yet, but if it works, then we need too write at least one test for these. (I'm willing too do so after this gets merged before the release.)
@soupman99 When do you think you'll be able too make these changes? We're hoping too see this in the release coming this week.
I don't know if I'll be able to make the changes by then.
-Justin youn2493@gmail.com 718.679.3231
On Sun, Nov 29, 2015 at 5:35 PM, Cody B. Daig notifications@github.com wrote:
@ilanbiala I left some more line comments that need too get addressed. The commits also need too be squashed. I haven't tested the code yet, but if it works, then we need too write at least one test for these. (I'm willing too do so after this gets merged before the release.)
@soupman99 When do you think you'll be able too make these changes? We're hoping too see this in the release coming this week.
Reply to this email directly or view it on GitHub: https://github.com/meanjs/generator-meanjs/pull/145#issuecomment-160481546
@codydaig is this PR still relevant or are we going to have this functionality with @UndeadBaneGitHub's PRs?
@ilanbiala I haven't PRed full CRUD generator, so, I suppose, this is relevant. However, tomorrow I plan on fixing the remaining issues with my remaining PRs and can PR CRUD module, built like my other generators for consistency.
@UndeadBaneGitHub we still need a CRUD generator, right?
Yep. I have a strong feeling I will write it sooner, than existing PR would get fixed
On Saturday, 23 January 2016, Ilan Biala notifications@github.com wrote:
@UndeadBaneGitHub https://github.com/UndeadBaneGitHub we still need a CRUD generator, right?
— Reply to this email directly or view it on GitHub https://github.com/meanjs/generator-meanjs/pull/145#issuecomment-174109077 .
@UndeadBaneGitHub that would be great.
@UndeadBaneGitHub this can be closed, right?
@ilanbiala yup, it can be.
I'm pretty new at github but did some work on the generator templates. If there's something I need to change please let me know. Hopefully part of this is usable.
All components needed to create a full CRUD module using
yo meanjs:crud-module <moduleName>