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

Changes 'config' key to point to latest parsed config file instead of only $PWD/.namerc if it exists #34

Closed
wants to merge 1 commit into from

Conversation

dbkaplun
Copy link

No description provided.

@dbkaplun dbkaplun changed the title Changes 'config' key to point to latest parsed config file instead of only $PWD/.slaprc if it exists Changes 'config' key to point to latest parsed config file instead of only $PWD/.namerc if it exists Nov 13, 2014
home ? cc.json(join(home, '.' + name, 'config')) : {},
home ? cc.json(join(home, '.' + name + 'rc')) : {},
cc.json(local),
local ? {config: local} : null,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the config key in question.

@dbkaplun
Copy link
Author

Hey @dominictarr, could you take a look at this PR? Thanks for making such a useful library!

@dominictarr
Copy link
Owner

sorry I missed this.
can you describe why this is useful and how you use it?

I do not merge pull requests that do not explain their context. There are a lot of people that use this,
and thus alot of code that I could break if I merge something badly. It's very important that I understand the WHY of a given change.

@dbkaplun
Copy link
Author

@dominictarr sure!

In the current iteration of rc, if rc('name').config exists, it means that ./.namerc exists. Now rc('name').config will be the path to any file that rc uses as config -- not just ./.namerc.

I am using this in slap to detect whether a user configuration file exists. If it does not exist, it displays a first-run message, and creates the file.

@dominictarr
Copy link
Owner

ah, okay this solves a problem similar to #33 @achingbrain @mikermcneil what do you think about this approach. It's quite simple, not a breaking change, and looks like it solves the problem too.

@achingbrain
Copy link

Looks like it would only report the last config file loaded. E.g. if I were to do:

$ config=/path/to/fileA node foo.js --config=/path/to/fileB

only /path/to/fileB would be reported even though it would have merged the config in /path/to/fileA and /path/to/fileB.

It would be better to report all loaded files rather than just the last one because the options in the files are merged not overwritten.

It also overwrites the config property of the returned object. If a user has a config section of their config (it's a crazy world, why not?), they don't any more.

@dbkaplun
Copy link
Author

@achingbrain the current version of rc creates a config key that behaves like it does in this PR, except only checking for $PWD/.namerc. So users should already expect the config key to be overwritten.

P.S. I agree that it would probably be better to report all loaded files rather than the last one. I wrote it this way to stay as backwards-compatible as possible. I'll be happy to change the PR if we can agree to change the config key to store an array rather than a string.

@dbkaplun
Copy link
Author

@dominictarr what do you think?

@dominictarr
Copy link
Owner

@secrettriangle hmm, I'm thinking that overwriting a --config option with an array is surprising. it would be okay if config is set to the top most file in the merge stack... normally there are not many layers.

Maybe there are some situations where you really do want full array... but I'd rather leave that feature until some one actually needs it, rather than try to guess it. Oh, just realized that was what @achingbrain has,
but @achingbrain to you really have more than one config file very often?

@achingbrain
Copy link

@dominictarr Yes, pretty much every time. The use case is: ship a config file with your app that contains all the options fully documented, then the user can override a subset of those options in a second config file.

@achingbrain
Copy link

FWIW I agree, swapping a string for an array seems a bit odd, that's why my PR added a method to return the array of loaded config file paths instead of overwriting existing keys.

@dominictarr
Copy link
Owner

@achingbrain okay this is interesting.
Can you show me an example of how you use rc with your defaults file? Rc has a place to set defaults but it sounds like you are doing it differently? Where do you put the defaults file?

@dbkaplun
Copy link
Author

dbkaplun commented Mar 1, 2015

I've rebased this PR and added the configs option to make everybody happy. See #44. Closing.

@dbkaplun dbkaplun closed this Mar 1, 2015
@dominictarr
Copy link
Owner

@achingbrain can I hear your side of the story on how you do defaults and files? else I will merge @secrettriangle's pr: #44

@achingbrain
Copy link

The configs property satisfies my need, so merge away. I think the config and configs properties should be non-enumerable though (more generally, any properties not loaded from a config file should be non-enumerable) - it makes storing in-app-edited config hashes in files a touch simpler.

@dominictarr
Copy link
Owner

@achingbrain that is an interesting point, though that would probably be a breaking change and it would be pretty hard to tell what it would break. It's another discussion, anyway.

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

Successfully merging this pull request may close these issues.

3 participants