-
Notifications
You must be signed in to change notification settings - Fork 43
Conversation
1 similar comment
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.
Looks great! I had a few questions. Also, can you write tests for getPrefix? I'd really like to not regress coverage on the repo. If not, I'll write some later, as I plan on turning on the --100
option for tap
at some point.
} | ||
} | ||
|
||
module.exports._fileExists = fileExists |
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.
Is there a reason to export this?
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.
unit testing
}) | ||
} | ||
|
||
module.exports._isRootPath = isRootPath |
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.
Same question as above.
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.
same answer as above
bin/cli.js
Outdated
main(parseArgs()).then((details) => { | ||
console.log(`added ${details.count} packages in ${ | ||
details.time / 1000 | ||
new Installer(parseArgs()).then(details => { |
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.
Should this be .run().then(...)
? Does this mean the tests don't currently exercise the CLI portion?
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.
Correct!
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.
oh yeah. I fixed this in a different branch. Fix't
Shucks, I wish I knew about tacks before making my own, essentially. Looks good! |
console.log(`added ${details.count} packages in ${ | ||
details.time / 1000 | ||
new Installer(parseArgs()).run().then(details => { | ||
console.error(`added ${details.pkgCount} packages in ${ |
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.
Error on success?
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.
Just writing it to stderr because it's not really meant as user data. Just informative side-stuff.
This rewrites the toplevel installer such that it uses object+method style instead of a bunch of toplevel functions. This is mainly useful so we can keep installer state over time: for example, I plan on using this to keep a "logical tree" around that I can use so scripts run in their logical dependency order.
As a bonus, this also pulls in
get-prefix
so we calculate the installer prefix using the same logic as npm.