yeoman / generator-gruntplugin

Create a gruntplugin module with yo, including Nodeunit unit tests.
http://yeoman.io
MIT License
35 stars 12 forks source link

Do not generate package.json from template. #3

Closed cowboy closed 10 years ago

cowboy commented 11 years ago

Instead, serialize a data object into a pretty-printed JSON string, and generate the package.json file from that.

Because, as it stands right now, When I specify an author of "Cowboy" Ben Alman it totally breaks things.

shama commented 11 years ago

It used to do this but that was changed (along with everything else I wrote for some reason?). See https://github.com/yeoman/generator-gruntplugin/commit/069f175cac9e2f532fad1e46247125c8efb33cd7#L0L191

addyosmani commented 11 years ago

Sorry, @shama. That's completely our fault. We'll work to get this resolved shortly.

@stepenplusplus some of the changes made in 069f175cac9e2f532fad1e46247125c8efb33cd7 to resolve exceptions in Grunt plugin scaffolding reverted some important updates we need to bring back in based on grunt-init-gruntplugin. See 24af108ff843cc875ed09cd0a90dbcf25be5cd57 for where @shama first started bringing these in.

Do you think you'll have time to review the diffs and see what we can do about this? If not, I'll try to prioritise some time on this after 1.0 ships.

We'll be working more closely together with the Grunt team on evolving generator-gruntplugin back to the level of functionality init used to offer and any help at all would be massively appreciated.

stephenplusplus commented 11 years ago

Of course, I'll take a look as soon as I can.

stephenplusplus commented 11 years ago

First, of course, I'm sorry if I blew out any of the work you did. Coming to this repository, it was hard for me to detect exactly what the game plan was. I thought I was taking a Grunt thing, and making it into a Yeoman thing. More specifically, using the Yeoman prompts and the Yeoman templating system/conventions.

I'm happy to fix anything I busted, but it would help me a lot to know what specifically should be brought back. And, better yet, if my "Make it work!" commit was actually making it not work, should we just roll back?

addyosmani commented 11 years ago

I recall the reason we merged in 'Make it work' being that the existing build wasn't functioning with the latest version of Yeoman at the time. How would this sound as a plan for the next release? In branch:

This will give us a generator that has @shama's modifications but is up to date and just works out of the box.

@stephenplusplus do you think you might have time to try getting this done?

addyosmani commented 11 years ago

@cowboy I updated this generator to support quoted strings in the author field and brought it inline with changes to other generators more recently. Testing it with a like-for-like transcript from grunt-init-gruntplugin it appears to now support the same input that project did.

I'll be doing the same for all of the other grunt-init template conversions shortly. My apologies for us dropping the ball on not shipping those in a completely stable state from the get-go. I accept responsibility for that.

More notes if interested:

I initially tried just using prompts.packageJSON() as per @shama's original PR, however found that it wasn't able to pick up all of the prompt fields. Yeoman generally favours using templates over inline JSON stringification so I'm string encoding the author input and extending the generator to input all the same fields grunt-init-gruntplugin did.

cowboy commented 11 years ago

FWIW, it's a mistake to generate JSON via anything other than a JSON serializer. Period.

cowboy commented 11 years ago

See http://stackoverflow.com/a/1732454/142339 for more.

addyosmani commented 11 years ago

I'll take another look at just JSON serialising prompt input shortly. It would at minimum avoid this issue cropping up on other fields.

hueitan commented 10 years ago

Does this a still issue on the current latest version ?

eddiemonge commented 10 years ago

It should be fixed as of 0.0.6

--edited. s/shouldn't/should be/

addyosmani commented 10 years ago

Yeah. This was fixed.

hueitan commented 10 years ago

Ok thanks @eddiemonge @addyosmani