uber-node / zero-config

A zero configuration configuration loader
MIT License
116 stars 10 forks source link

strictGet method and tests #12

Closed jskelcy closed 9 years ago

jskelcy commented 9 years ago

There are a lot of tests, let me know if I need to add this into more tests

Raynos commented 9 years ago

Let's do this differently. Have it be strict by default have have an { loose: true } configuration argument to make it loose. Do not add a new method, just make .get() strict.

jskelcy commented 9 years ago

Taking a look at this now and not seeing a good place to add the loose property. The opts.seed seems like it was meant to be optional and the opts.default doesn't seem right either.

I have a local branch where I have everything working with something called opts.options (stupid name but just kind of a proof of concept). It feels a little janky around some of the blacklist test but its not too terrible. Let me know if you have a preference as to where you would like this option to be added, otherwise I can just add it to opts.seed and update the PR and you can let me know what you think.

Raynos commented 9 years ago

@jskelcy try opts.loose ;)

jskelcy commented 9 years ago

yeah, I was trying to play into the fact that config chain is weird about taking non-objects. But yes opts.loose will work. Looks a little funny passing it into the config chain but all in all not so bad.

Raynos commented 9 years ago

@jskelcy loose doesnt have to go into configChain.

Just pass it as a second argument to config-wrapper

lxe commented 9 years ago

What is the purpose of multiple modes? Also, I don't think throwing an error on get() is a very good idea. A lot of get()s might happen in non-initialization code, and wrapping each one of them in try-catch is A Bad Thing:tm:

If we're proceeding with this, why not the conventional .get(key, callback(err, data)) instead?

Raynos commented 9 years ago

@lxe the point is that getting a non-existant key is a bug.

You throw uncatchable exceptions for bugs.

The loose mode exists for back compat for people that want the old semantics.

Raynos commented 9 years ago

lgtm. @jskelcy #shipit

rf commented 9 years ago

@jskelcy why haven't you rebased this yet damn