feat(extract): add 'safe mode' via extractOverwrite option #82
Conversation
|
oops my VSCode settings overrode your style settings, changing some of that back |
extract.js
Outdated
| @@ -85,3 +89,22 @@ function cleanUpCached (dest, cachePath, integrity, opts) { | |||
| cacache.rm.content(cachePath, integrity, opts) | |||
| ) | |||
| } | |||
|
|
|||
| function checkOverwrite (extractOverwrite, spec, dest) { | |||
| return new BB((resolve, reject) => { | |||
There was a problem hiding this comment.
You can turn this bit into something like:
if (extractOverWrite) {
return BB.resolve()
} else {
return readdirAsync(dest).then(() => {
const err = ...
throw err // Promises catch throws and turn them into rejects if inside a promise handler scope
}).catch({code: 'ENOENT'}, () => {}) // catching will automatically resolve
}
extract.js
Outdated
| opts.log.silly('pacote', `data for ${opts.integrity} not present. Using manifest.`) | ||
| return extractByManifest(startTime, spec, dest, opts) | ||
| } | ||
| checkOverwrite(opts.extractOverwrite, spec, dest) |
There was a problem hiding this comment.
You should add a return here, so that this promise and its sub-promises become the return value for extract.
|
I added some commentary since you mentioned you hadn't played with Promises before. Hopefully it helps :) |
|
Oh and also thank you! This is looking great so far 🙆 |
|
@zkat commentary greatly appreciated :) review fixes and tests coming soon! |
|
Hey! Sorry, I missed your last update! This is looking great so far. The last thing I'd like to ask is whether you'd be willing/able to write a test for this: just a test that calls If this seems like too much, that's fine -- I've been slacking a lot on tests myself. And don't worry about the conflict! I can resolve that for you 👌 |
zkat
force-pushed
the
latest
branch
2 times, most recently
from
dda8238 to
56c4cb0
Compare
|
@zkat can we merge this someday? I was looking for an ability to do this today. |
|
whoa. totally forgot i had this PR open. let me take a look today or
tomorrow and clean this up a bit.
…On Tue, Feb 26, 2019 at 07:14 William Bernting ***@***.***> wrote:
@zkat <https://github.com/zkat> can we merge this someday? I was looking
for an ability to do this today.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#82 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB0keyIYm4TDVgt09KfIE7QEEUsrJSAoks5vRSUxgaJpZM4NMXpD>
.
|
|
If we want to make sure not to break anything, I think the safe mode should be opt in. |
More or less a WIP, first round without tests. I've never worked with Promises IRL before, my only Promise experience is via API clients on the front-end and my node experience is limited, so I want to know what I did wrong here before digging into tests.
Adds the 'safe mode' outlined in #73, requiring a config option
extractOverwriteto overwrite an existing directory, otherwise aborting the extract operation@zkat