Added a separate default config option to allow for shared + custom config. #16

Merged
merged 4 commits into from Dec 5, 2013

Projects

None yet

2 participants

@Pradeek
Contributor
Pradeek commented Nov 27, 2013

No description provided.

@Pradeek Pradeek referenced this pull request in stefanpenner/ember-app-kit Nov 27, 2013
Closed

Easier upgrading #256

@jgallen23
Member

interesting. what's the use case?

@jgallen23 jgallen23 commented on an outdated diff Nov 28, 2013
lib/load-config.js
+ if(options.defaultPath !== null) {
@jgallen23
jgallen23 Nov 28, 2013 Member

lots of duplicate code here. I would think there should be a single function that takes the directory and returns the full object. Then you just do the merge at the end

@Pradeek
Contributor
Pradeek commented Nov 28, 2013

Primarily for boilerplate solutions like the Ember App Kit, I needed a way to separate the grunt options given in the app kit from my own customisations. This separation is needed when we need to update the app kit and I need to maintain my own customisations.

Reg code duplication, will clean up and push later today.

@jgallen23
Member

Sounds good. I think it should be called configDir and customConfigDir. Also, make sure to add tests

@Pradeek
Contributor
Pradeek commented Nov 28, 2013

I left configPath as it is and added a defaultPath to maintain backwards compatibility, as a lot of projects might already be using this. Also, I think of it more so of allowing an optional default rather than an optional customizable config, if that makes any sense.

@Pradeek
Contributor
Pradeek commented Nov 28, 2013

I've separated out the simplemocha config as an example on how to set this up. Any suggestions on how to do this (and test this) better?

@Pradeek
Contributor
Pradeek commented Dec 4, 2013

anything else to be done here?

@jgallen23
Member

It looks like I can't auto merge anymore. Can you look into that? Then I'll merge this in.

@Pradeek
Contributor
Pradeek commented Dec 4, 2013

Merged the latest changes and tested it.

@jgallen23
Member

k. will merge this in today. I'm going to separate the grunt register part from the actual logic so it's easier to test this.

@Pradeek
Contributor
Pradeek commented Dec 5, 2013

Awesome

@jgallen23 jgallen23 merged commit 2068d8f into firstandthird:master Dec 5, 2013
@jgallen23 jgallen23 added a commit that referenced this pull request Dec 5, 2013
@jgallen23 jgallen23 Revert "Merge pull request #16 from Pradeek/separate-default-config"
This reverts commit 2068d8f, reversing
changes made to 9a69976.
d037318
@jgallen23
Member

I reverted this because I'm going to do some refactoring and bring this back in later. might take me a day or so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment