Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A proposed solution to the problem of overwriting configurations #6

Closed
Merott opened this issue May 16, 2014 · 5 comments
Closed

A proposed solution to the problem of overwriting configurations #6

Merott opened this issue May 16, 2014 · 5 comments

Comments

@Merott
Copy link

Merott commented May 16, 2014

As you mentioned in the Wiki, currently there is no functionality to completely overwrite a configuration object or value. I'm proposing a solution, which if you accept, I'll happily submit a PR for.

I'm proposing the following syntax for completely overwriting/replacing a configuration:

"tasks": {
   "karma": {
      "single-run": {
         "(replace)browsers": [
            "Firefox"
         ]
      }
   }
}

A small change in deepmerge.js makes this possible: Merott@9ff0818

Object.keys(src).forEach(function (key) {
    if(key.indexOf('(replace)') === 0) {
        dst[key.substr(9)] = src[key];
    }
    else if (typeof src[key] !== 'object' || !src[key]) { ...

Do you see any problems with that approach?

@joshdmiller
Copy link
Member

You're right - this is much cleaner than my ideas. We would obviously have to create a fork of the deep merge library (it's in there temporarily because the author can't push a bug fix needed by Warlock until his npm permissions get reset).

The only thing I might change is the syntax itself. Maybe something shorter like just "r" or "ow" (overwrite). We could also just pick a shorthand character sequence like ^, which is unlikely to be used as a key:

{
"tasks": {
   "karma": {
      "single-run": {
         "^browsers": [ "..." ]
      }
   }
}

Anyway, great idea!!!

@Merott
Copy link
Author

Merott commented May 16, 2014

I prefer ^

I also originally considered using ^, and the only reason I didn't use it was that I thought being a little more explicit with (replace) would make the syntax easier to understand. I have no real objection to ^ though, so I'd be happy with either of them.

@joshdmiller
Copy link
Member

I prefer ^ too. I normally opt for clarity too, but in this case the aesthetics on the shorthand win me over. It's just easier to read for me. If you'd like to submit a PR, that'd be awesome!

However, we probably don't want to just change deepmerge.js as it'll be harder to merge upstream changes. Perhaps we should fork it and create a warlock-deepmerge library we can throw on npm. We can then add a test case for that change too since we don't have unit testing on Warlock proper - yet (see #4). Then we get the added bonus of removing deepmerge.js from the Warlock repo.

What do you think?

@Merott
Copy link
Author

Merott commented May 16, 2014

Good plan. I'll write a test case for it and submit a PR to warlock-deepmerge.

@joshdmiller
Copy link
Member

warlock-deepmerge has the change and all is groovy.

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

No branches or pull requests

2 participants