feathersjs-ecosystem / configuration

[MOVED] A plugin for configuring a Feathers application
https://github.com/feathersjs/feathers
MIT License
28 stars 15 forks source link

Support modules as configuration files. #13

Closed wkw closed 8 years ago

wkw commented 8 years ago

Node require('filename') w/out the file extension will look for either filename.js first, or if not found, filename.json. This commit expands feathers-configuration to support default or settings files which can generate derived configuration properties. All support for path expansion and environment variables remains intact.

Closes #12

ekryski commented 8 years ago

Thanks for the PR @wkw! 😄 This makes total sense and it looks good to me.

@daffl you want to be the one to sign off and release this? If you don't get to it by tomorrow evening then I will.

daffl commented 8 years ago

Looks like this will be completely backwards compatible so this is a great feature, thank you! Only one question about why the JSHint command had to be changed once that is cleared up we can make the release.

wkw commented 8 years ago

@daffl You can revert that line, or find a better way, but it was the solution that worked for me. the problem was... as I started changing the code, jshint was unhappy with a couple things test_environment not being camel'd, and one other thing. But I never got that feedback if I ran npm test. I'd just get a node spew of ERR lines but no clues.

So I switched to running jshint alone to find the problems. But that shouldn't be necessary. I just dont know how to make the jshint ... && ... show the errors when firing the tests. My googling more or less came up with that mod you're questioning.

There is surely a better way to get jshint's warnings/errors than that. This is not so unlike what I mentioned on the generators PR, which you promptly dealt with: https://github.com/feathersjs/generator-feathers/commit/f006e4dd4d6ca63e551d0a740d1a864c71c1b60f

Could it be my environment? (and yes, I'm on Mac OS X)

wkw commented 8 years ago

OK, I just re-ran the tests w/out the 2>/dev/null and even though I get the npm ERR spew, I do see now the jshint complaints:

> feathers-configuration@0.2.2 jshint /Users/wkw/code/javascript/feathers-configuration
> jshint src/. test/. --config
test/config/testing.js: line 14, col 10, Identifier 'foo_bar' is not in camel case.
test/config/testing.js: line 14, col 17, Strings must use singlequote.
2 errors

Sorry for that. But swear I didn't see those coming out last night at 3am... :-)

I'll resubmit with the change if you want. lemme know.

ekryski commented 8 years ago

@wkw yes if you can revert that 2>/dev/null change that would be great! We'll just merge this with the commits squashed so no need to cherry pick or anything.

wkw commented 8 years ago

@ekryski fix is up. Are you able to squash those two commits at merge time?

ekryski commented 8 years ago

@wkw yup. @github added that feature fairly recently. Thanks for implementing that! I've published v0.2.3 with your updates.