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

Clarification on the defaults argument needed #76

Open
leschekfm opened this issue Mar 9, 2016 · 4 comments
Open

Clarification on the defaults argument needed #76

leschekfm opened this issue Mar 9, 2016 · 4 comments

Comments

@leschekfm
Copy link

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
Copy link
Owner

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 f7fe129 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
Copy link
Author

OK that explains a lot ;)

@leschekfm
Copy link
Author

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
Copy link

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

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

No branches or pull requests

4 participants
@dominictarr @leschekfm @legodude17 and others