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

Cache loaded config and merge it with defaults #387

Merged
merged 1 commit into from
Jul 4, 2016
Merged

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jun 8, 2016

Fixes #249

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jun 8, 2016
}

Helper.config.host = program.host || process.env.IP || Helper.config.host;
Copy link
Member Author

Choose a reason for hiding this comment

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

... Do we want to keep ENV here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really see a use for it. I would get rid of it. I think the only cases we should use environment variables is for things that can't be in the config file (so, the config path).

@xPaw xPaw force-pushed the xpaw/config branch 2 times, most recently from dc16643 to b0431af Compare June 12, 2016 15:14
@xPaw xPaw added this to the 2.0.0 milestone Jun 12, 2016
@AlMcKinlay
Copy link
Member

I'm not really sure why you needed to change getConfig, surely you could have stored config as a private variable, and getConfig just returned config. That would have meant a lot less code changes.

@xPaw
Copy link
Member Author

xPaw commented Jun 13, 2016

@YaManicKill There are not enough getConfig calls to warrant keeping it, especially in places where only a single variable is checked.

@AlMcKinlay
Copy link
Member

@xPaw At least 6 of the 9 files that are edited are edited only because you removed the function. It's not exactly like a getter is less efficient, anyway. What's the point in changing that?

@xPaw
Copy link
Member Author

xPaw commented Jun 13, 2016

@YaManicKill Some of the files were changed to remove default fallbacks (e.g. link.js or userLog.js).

Other files kept it's own stored cached configs (like server.js and chan.js). For consistency I decided to make it a public variable, instead of a function call.

@maxpoulin64
Copy link
Member

Other files kept it's own stored cached configs (likeserver.js and chan.js). For consistency I decided to make it a public variable, instead of a function call.

Actually since this is JS it doesn't matter much as objects are always references, so either way you get the same thing in the end.

Another option to make this all shorter could be to do var config = require("helper").config; since most places don't use Helper functions at all.

@maxpoulin64
Copy link
Member

Looks fine to me, so I will 👍 and we can improve later. I'm not a fan of using "Helper.config" everywhere and would rather have var config = require("helper").config;, but it's not enough to block this.

@AlMcKinlay
Copy link
Member

Fine, whatever. I don't really care enough, I just didn't see the point. So, whatever. 👍 and merging.

@AlMcKinlay AlMcKinlay merged commit 9c07f2b into master Jul 4, 2016
@AlMcKinlay AlMcKinlay self-assigned this Jul 4, 2016
@xPaw xPaw deleted the xpaw/config branch July 4, 2016 15:16
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Cache loaded config and merge it with defaults
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants