Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

Merge default options with config files if property not specified #58

Merged
merged 2 commits into from
Sep 22, 2013

Conversation

justcfx2u
Copy link
Contributor

At present, most mosca config file options do not specify a logger object.
That makes them break when, for example, trying to use verbose output mode.

Example mosca.conf:

module.exports = {
  port: 1883,
  backend: {
    type: "redis"
  }
};

Then run:

mosca -c ./mosca.conf -v

Will dump an error like:

$ mosca -c ./mosca.conf -v

/usr/lib/node_modules/mosca/lib/cli.js:96
      opts.logger.level = 30;
                        ^
TypeError: Cannot set property 'level' of undefined
    at /usr/lib/node_modules/mosca/lib/cli.js:96:25
    at cli (/usr/lib/node_modules/mosca/lib/cli.js:232:12)
    at Object.<anonymous> (/usr/lib/node_modules/mosca/bin/mosca:3:22)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:901:3

This patch makes sure all "default" options are set to default values (merging default options with those specified in a config file) instead of outright replacing them, allowing configuration files to override only what is specified, while having a reasonable guarantee that the rest of the code which may depend on a default value can continue to do so. It also has the side-effect of making the current examples work better.

@mcollina
Copy link
Collaborator

Thanks! Could you please add a simple unit test in 'test/cli_spec.js' to test this behaviour? So we won't break it in the future.

@justcfx2u
Copy link
Contributor Author

Done. Even though the test case is rather specific it manages to illustrate that the above patch is doing something desirable.

@mcollina
Copy link
Collaborator

That's perfect! Thanks a million.

mcollina added a commit that referenced this pull request Sep 22, 2013
Merge default options with config files if property not specified
@mcollina mcollina merged commit 2c58788 into moscajs:master Sep 22, 2013
@mcollina
Copy link
Collaborator

Released as 0.12.1.

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

Successfully merging this pull request may close these issues.

2 participants