Fixes #42 use configPath instead of hard-coded config #100
Conversation
README.md
Outdated
| @@ -33,7 +33,7 @@ When running with `parallel` set to true, the final reports can be merged by usi | |||
|
|
|||
| ## Configuration | |||
|
|
|||
| Configuration is optional. It should be put in a file at `config/coverage.js`. | |||
| Configuration is optional. It should be put in a file at named `coverage.js` inside your ember app config folder (defaults to `config/`). | |||
There was a problem hiding this comment.
I'd prefer to keep this README line unchanged. 99% of our users will be using config/coverage.js, and I think rewording like jisnjust makes it hard to grok for the vast majority use case. I think a parenthetical aside or a second sentence mentioning that we honor configPath is enough...
There was a problem hiding this comment.
Thanks for taking a look at this! I've reworded, let me know if the new message is more inline with what you have in mind.
index.js
Outdated
| attachMiddleware(app, { root: this.project.root, ui: this.ui }); | ||
| attachMiddleware(app, { | ||
| root: this.project.root, | ||
| configDirName: path.dirname(this.project.configPath()) |
There was a problem hiding this comment.
Why are we no longer passing in ui?
Can you put the config path invocation above here as a variable declaration (then just pass it through here)?
There was a problem hiding this comment.
On second thought, I think it would be better to simply pass the project down into the middleware and have our config util function accept the project and avoid the path munging throughout the rest of the system.
There was a problem hiding this comment.
-
I removed
uibecause it was not being reference from the middleware so the parameter was unnecessary. -
I changed the config code to now take in the full
projectand construct the path inside.
|
@kategengler - Any objections? |
There was a problem hiding this comment.
Only opposed to passing the project object around, would prefer to keep defined inputs to things outside of index.js, but I could be convinced otherwise.
Thanks for tackling this!
@rwjblue I am otherwise 👍 if you think it's ok to be using configPath (since it is marked private)
lib/config.js
Outdated
| * @returns {Configuration} configuration to use for project | ||
| */ | ||
| function config(root) { | ||
| var configFile = path.join(root, 'config', 'coverage.js'); | ||
| function config(project) { |
There was a problem hiding this comment.
I'd prefer to continue to pass in the config path, rather than pass around the project (I'd like to keep usage of ember-cli api contained to index.js as much as possible)
There was a problem hiding this comment.
I think this makes sense because I could still keep the logic to construct the path to coverage.js inside config like @rwjblue suggested but also not take in the whole project object.
I can make this change if there are no objections.
There was a problem hiding this comment.
👍 - Thanks for iterating on this with us
There was a problem hiding this comment.
@kategengler @rwjblue No problem! I've updated with the feedback received. Let me know if you notice anything else and thanks for taking the time to review this
lib/attach-middleware.js
Outdated
| @@ -12,12 +12,12 @@ function logError(err, req, res, next) { | |||
| next(err); | |||
| } | |||
|
|
|||
| module.exports = function(app, options) { | |||
| module.exports = function(app, project) { | |||
There was a problem hiding this comment.
I think this should still take options and get a configPath and root as properties
|
Looks great, thanks for making those changes @eddie-ruva |
|
@kategengler, any estimate when will this be released in the next version? |
|
Released now as v0.3.12. |
On this PR I'm trying to tackle the issue brought up in #42 by using
configPathinstead of the hard coded config path value.According to the ember-cli docs
configPathis private but I couldn't figure out a better/public way to determine this value. If anybody knows of a better way to make this change I'll be happy to integrate the feedback.