search

dominictarr / rc

The non-configurable configuration loader for lazy people.
Other
1.02k stars 97 forks source link

Clarification on the defaults argument needed #76

Open leschekfm opened 8 years ago

leschekfm commented 8 years ago

From the documentation I would assume that the second argument for rc should always be an object.

Reviewing the code I noticed that there is also a handling for the default param being a string. I would assume that this was meant to be an option to pass a stringified JSON object as the defaults variable is handed to utils.json() then.

Looking through utils.json() the defaults var (which has to be a string at this point) is passed to utils.file(). This leads me to the conclusion that the second argument could also be a path to a .json or .ini file?

If that is right, I think it would be a good idea if this was documented.

dominictarr commented 8 years ago

this is a news to me actually! It should be an object, maybe it snuck in via some pull request. Oh it seems that I wrote that line https://github.com/dominictarr/rc/commit/f7fe1296402cae56214a89a053c0d728072b9271 and called it "graceful defaults" I guess I meant do something sensible with a string default in that position.

Though, the thing past @dominictarr wasn't taking into account is that quite likely putting a string in that position was an accident, and so this makes it passively pass but not do what you expected. Since this was a surprise to me, and undocumented, I think we should remove that, and throw an error if the defaults object is anything but a {} object

leschekfm commented 8 years ago

OK that explains a lot ;)

leschekfm commented 8 years ago

I improved the type check and wrote some tests for variable types that could cause problems. Please have a look at my pull request @dominictarr

legodude17 commented 8 years ago

I actually think that the passing a file path for default args is a good idea and should be documented.