Merge pre-existing package.json with file generated by Cloverfield

halhenke commented 9 years ago

Re issue #23

Tried to make it fit the pre-existing code structure as much as possible.
There is some "intelligence" to the merging from the package-merge package rather than a straight object merge - keywords field will be merged without duplicates etc. Might want to take a quick look at that package to see if its desired behaviour:
- https://github.com/izaakschroeder/package-merge/blob/master/lib/merge.js

ericelliott commented 9 years ago

Tests fail due to a missing package.

halhenke commented 9 years ago

Yeah, seems the guy who maintains package-merge didnt declare lodash as a dependency.

Its passing on my machine seemingly because its a dependency of precommit-hook thats been spat into node_modules by npm 3 as far as i can tell.

Not sure what the best approach is - theres not a ton of functionality in that package but i thought it would be nice to have. Could fork it and make a PR and in the meantime use the fork or do a simpler { ...origPkg, ...genPkg} type merge?

nkbt commented 8 years ago

Hi, @halhenke can you please update PR so I can merge it? thanks :ok_hand:

halhenke commented 8 years ago

OK for sure - sorry thought i already had - must be losing it... :-(

nkbt commented 8 years ago

Sorry, for the delay, @halhenke I've just checked the new generation process. Package.json is merged respecting existing values. Other files not, though. Like README.md has new values, though package.json still has old ones.

I am not sure about intended behaviour here. I can suggest that when we merge package.json we can see which values have been changed and replace them accordingly in other files too. But not touching values that should not be changed.

Alternative (and much easier) solution could be - read package.json to get "suggested" values for the second run. Then either I just hit "Enter" and use those default ones - or enter new ones - does not matter. Replace all things as usual.

Honestly after playing with it a bit I'd rather go for the second solution.

Related to #44 by the way.

ericelliott commented 8 years ago

:+1:

halhenke commented 8 years ago

Hey sorry for delay in getting back to you Nik - busy few days,

Didn't realise we wanted to merge README also - was only thinking of package.json.

I also like the second idea - at least it makes people aware of whats happening. Will rework it and see if i can come up with something that deals with other files - think there could be some hard to deal with edge cases - but otherwise will go for the user prompt option.

Did you want me to try and work in suggestions from #44 also (i got the impression the other guy didnt want to)? Dont mind.

nkbt commented 8 years ago

Awesome! #44 looks like duplicate to me then.

halhenke commented 8 years ago

Cool! :+1:

ericelliott commented 8 years ago

What's going on with this?

halhenke commented 8 years ago

Sorry, sort of got lost in Christmas stuff - have a couple of free days, will try to finish it off.

ericelliott commented 8 years ago

Cool. Thanks!

halhenke commented 8 years ago

OK this thing has been dragging on for a while (apologies) would love to wrap it up

Not entirely happy with what i've done (it feels a bit hack-ish) but wanted to check its the direction we wanted.

Anyway if a package.json file is found that exists:

grab author/email/github from package.json & use as defaults
read dependecies, devDependencies and scripts fields and add questions whether the user wants to merge them into the template package.json (could easily add other fields like peerDependencies if you like)

Feels a bit inelegant because its not using the same handlebars template/vars method to modify most fields - though we do now have author/email variables etc that will now be used in other templates also.

Could use more tests. Anyway just wanted to get any feedback/suggested changes when you guys have time.

halhenke commented 8 years ago

Hey guys - just wanted to say - if this PR just missed the mark or isnt working out I wont be offended if you just want to kill it or anything. Its all pretty fuzzy for me at this point anyway.

ericelliott commented 8 years ago

@nkbt Comments?

nkbt commented 8 years ago

Wow, that looks cool, pretty much package.json-specific merging. I wish github sends emails on PR updated. I'll try it on couple of existing projects and let you know asap!

ericelliott commented 8 years ago

@nkbt Any more thoughts?

halhenke commented 8 years ago

:disappointed:

I'll take a look at it either tonight or later this weekend.

cloverfield-tools / cf-package

Merge pre-existing package.json with file generated by Cloverfield #40