Support multiple configuration files. #67
Conversation
|
Thanks @sttk - I am currently wrapping up some contract work and should have time to review this soon |
| var fs = require('fs'); | ||
|
|
||
| var homedir; | ||
| if (os.homedir) { |
There was a problem hiding this comment.
|
@sttk this PR looks really cool but there's a ton of code in here. I made some cursory comments on ways to reduce the code and allow me to focus on the logic. I think another thing that would help would be to have tests written out instead of generated from the fixture you created. |
| @@ -2,7 +2,8 @@ language: node_js | |||
| node_js: | |||
| - "0.10" | |||
| - "0.12" | |||
| - "iojs" | |||
| - "4" | |||
| - "stable" | |||
There was a problem hiding this comment.
For consistency, I would rather see 5 here.
There was a problem hiding this comment.
I like to keep this as stable (I wish we could reference LTS instead of 4 also). This is the pattern used in all the gulpjs repos
|
@phated @jonschlinkert @tusbar Thank you all for reviewing and advising. I'll modify all of what you pointed out. @jonschlinkert I will use parse-filepath, is-absolute! @phated I welcome the further pointed out. And I expect a better way of testing. Thanks. |
|
@sttk awesome, thanks. Let me know when the updates are done. |
|
I've finished modifying the codes. I stopped using glob and |
| var isAbsolutePath = require('is-absolute'); | ||
| var parsePath = require('parse-filepath'); | ||
|
|
||
| function isWindows() { |
There was a problem hiding this comment.
this can't change, so it should probably just be a variable defined here var isWindows = process.platform === 'win32'
|
I've finished modifying! |
| this.emit('requireFail', attempt.moduleName, attempt.error); | ||
| } else { | ||
| this.emit('require', attempt.moduleName, attempt.module); | ||
| registerLoader(this.extensions, configPath, cwd, this); |
There was a problem hiding this comment.
I'd recommend moving the this from the last argument to the first. Maybe this function signature is getting too complex and should be changed to an object with each argument as a property. @tkellen thoughts?
|
@sttk except for my few notes and questions, this looks great! |
|
@phated Thank you for pointing out. I rebased and reflected changes of master. In addition, I modified something that you pointed out above and I could do immediately. |
|
@sttk sorry for the delayed review. This looks amazing. Can you believe that you reduced the PR to 300 lines of code (most of it being tests)? I'd love to get @tkellen and/or @jonschlinkert to review before I click the merge button. |
| }; | ||
| }; | ||
|
|
||
| // use rechoir to autoload any required modules | ||
| function registerLoader(eventEmitter, extensions, configPath, cwd) { |
There was a problem hiding this comment.
this should be in lib/register_loader.js. ideally it'd be unit tested, too!
|
Incredible work @sttk. If you have the appetite to address my final concern in this PR, awesome. If you'd rather do that as a separate PR after this is landed and published, I'm down with that too. |
|
PS: Thank you!! |
|
@tkellen Thank you for reviewing. I'll address what you pointed out in this PR. |
|
I've separated |
|
@sttk this looks great! Can you rebase one last time? I merged an update to the findup-sync module and it conflicts with some of your changes. Once you rebase, I'll merge this in the next couple of days (I'd like to make sure the findup-sync change isn't causing problems before bumping again). |
|
@phated I've rebased. |
|
@tkellen I'm looking to ship this today, you good with me doing that? |
|
|
|
@sttk thank you so much for your hard work on this! I'm going to be adding docs and then publishing this feature. |
|
Thank you very much!! |
|
This was just published as 2.3.0. Once again, amazing work. |
This is the PR about #8.
Please review.