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

Allow environment variable to override default config. #112

Merged
merged 1 commit into from
May 12, 2016

Conversation

cthrax
Copy link
Contributor

@cthrax cthrax commented May 11, 2016

While attempting to override the TS_NODE_PROJECT environment variable for use with gulp, which uses 'ts-node/register', I found that the environment variable did not actually override the default config. It does now. :)

@coveralls
Copy link

coveralls commented May 11, 2016

Coverage Status

Coverage remained the same at 88.119% when pulling c19803d on cthrax:master into 6422205 on TypeStrong:master.

@@ -130,7 +130,7 @@ const DEFAULT_OPTIONS: Options = {
*/
export function register (opts?: Options) {
const cwd = process.cwd()
const options = extend(DEFAULT_OPTIONS, { project: cwd }, opts)
const options = extend(DEFAULT_OPTIONS, opts)
Copy link
Member

Choose a reason for hiding this comment

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

Please just re-order the parameters here. We don't want cwd coming from two places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blakeembrey I don't think re-ordering will work. The testing that I did showed "extend" would overlay an "undefined" if provided. So if DEFAULT_OPTIONS was overlayed on top of {project:cwd} and no environment variable was set, then the options would end up with undefined in project. I could move it down into this method and say "if (!options.project) { options.project = cwd; }" but that seemed less straightforward than putting it in the initialization of DEFAULT_OPTIONS. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

You're absolutely right! Sorry I missed that yesterday. In that case, I think it's good 👍

@blakeembrey
Copy link
Member

Thanks for the catch, I only have the one comment.

@blakeembrey blakeembrey merged commit bcf196b into TypeStrong:master May 12, 2016
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