-
Notifications
You must be signed in to change notification settings - Fork 110
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
Use in-repo addon's "root" property for includes location #290
Use in-repo addon's "root" property for includes location #290
Conversation
# Conflicts: # .gitignore # index.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this definitely seems like the right direction! I'd like to land #289 first (to avoid that larger PR having too much rebase hell), but we should definitely get this in...
index.js
Outdated
typeof addon.root === 'string' | ||
? addon.root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why would addon.root
not be present? I would think it always would...
index.js
Outdated
let addonDir = | ||
typeof addon.root === 'string' | ||
? addon.root | ||
: path.join(this.project.root, 'lib', addon.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this seems super odd / not good. Assuming that an addon is in lib/some-name
is definitely broken.
OK, landed #289 so this is ready for a rebase. Note the testing system has changed a bit (not using |
…e-coverage � Conflicts: � .gitignore
…overage into in-repo-addon-paths � Conflicts: � .gitignore
Done, and might I add a very pleasant evolution to the testing story with #289. The hesitance of touching anything |
Thank you @mdeanjones! |
const fs = require('fs-extra'); | ||
const fixturify = require('fixturify'); | ||
|
||
class InRepoAddon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need this file anymore, right?
I have a question on how to exclude an in-repo-addon introduced into the coverage report by this PR. We have a folder structure like
And in our-app's package.json we specify the in-repo-addon with How can I exclude this in-repo-addon from the coverage report? I've tried even |
…code-coverage#290) Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
…code-coverage#290) Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
…code-coverage#290) Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
…code-coverage#290) Co-authored-by: Jones, Michael Dean (Contractor) <MDJONES@associates.nsf.gov>
The goal is to instrument in-repo addons that don't reside directly within
/lib
by using the addon's root property, if available.Given how long the current implementation has been around I think I might be missing some important context that makes this less viable than a quick glance would suggest.