[gh238] [bz5667423] support for multiple application configuration files #905

Merged
merged 6 commits into from Jan 11, 2013

2 participants

@caridy

Continuation of PR #898.

By specifying the list of files to aggregate with the application.json, developers will have the flexibility to split the application configuration into various files, e.g:

[
    {
        "settings": [ "master" ],
        "applicationConfigFiles": [
            "node_modules/devices/application.json",
            "relative/path/to/custom.json"
        ],
        "testKey1": "testVal1",
        "testKey2": "testVal2",
        "testKey3": "testVal3"
    }
]
@drewfish drewfish commented on the diff Jan 10, 2013
lib/app/addons/rs/config.js
+ * @method createMultipartYCB
+ * @param {array} paths list of files to load
+ * @return {YCB|undefined} return a YCB library object, or undefined if an error occurs
+ */
+ createMultipartYCB: function(paths) {
+ var p,
+ path,
+ config,
+ s,
+ section,
+ settings = {},
+ bundle = [];
+ bundle.push(this.getDimensions()[0]);
+ for (p = 0; p < paths.length; p += 1) {
+ path = paths[p];
+ config = this.readConfigSync(path);
@drewfish
Yahoo Inc. member
drewfish added a line comment Jan 10, 2013

This should use readConfigSimple() instead. The work to wire in YAML will update readConfigSimple() to also understand YAML files.

@caridy
caridy added a line comment Jan 10, 2013

At the moment, readConfigSimple does not provide that capability, and the multi-extension capability as well. Once that land it, we can change it I guess.

@drewfish
Yahoo Inc. member
drewfish added a line comment Jan 10, 2013

That's part of what I'm saying. Right now in develop, we should not be trying to support YAML. So, you should use readConfigSimple(). The work to support YAML will do the needful, whether that's update readConfigSimple() to readConfigSync() or not.

(I am working on the YAML support. readConfigSync() is going away, so calling it will cause a merge conflict of sorts. It's better to use the established API (readConfigSimple) than to use readConfigSync() which isn't used by anyone else and which is going away.)

Said another way, this pull request should -only- be focused on implementing multiple application.jsons, and not also trying to partially support YAML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drewfish
Yahoo Inc. member

Does this support application.json which includes application2.json which includes application3.json?

@caridy

@drewfish No, that's not supported. It developers want to keep control over what they have/use, there is not need for that. Also the error tolerance is important, since application.json is optional.

@caridy

@drewfish I will merge this so we can work on the PR #907 to accomodate the rest of the changes.

@caridy caridy merged commit eb4947e into yahoo:develop Jan 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment