Add callback argument to applyPatches patched option

[piranna]

This fixes #129

[piranna]

This one looks good !

Thanks! ^^

[kpdecker]

It's generally bad form to combine multiple features into one pull request. Please back out #131 from this as I don't want that in here.

WRT this exact feature, I'm worried about making this a breaking change. I'm not sure if it would be better to do something like patchedAsync or similar. Can you walk me through the use case that requires the patched callback to be async?

[piranna]

It's generally bad form to combine multiple features into one pull request. Please back out #131 from this as I don't want that in here.

I added it here since they were somewhat related. I can be able to remove it #131 is not open for discussion.

WRT this exact feature, I'm worried about making this a breaking change. I'm not sure if it would be better to do something like patchedAsync or similar.

Another option would be to check the number of arguments of the function to see if it has defined a callback (options.patched.length === 3) and if so use it async, and otherwise sync.

Can you walk me through the use case that requires the patched callback to be async?

On download-manager I'm using it to apply patches to Linux kernel, GCC and some other tools for NodeOS and write them back to the filesystem before start processing or compiling them, so I need both to be sure they have been fully written before using them and to capture any possible error when writting to manage it cleanly. I could be able to write them synchronously to the filesystem and using a try-catch, but all the module is fully async and this would be like an alien, and also it would became a bottleneck and decrease performance (I'm downloading, decompressing, patching and writting the files on the fly...).

[kpdecker]

Ok, I think that makes sense re: async w/ async. I don't want to go the route of checking the args length because that can also have unexpected conseqences, but I'm fine merging this and making it a major release after the changes for #131 are removed.

[piranna]

I don't want to go the route of checking the args length because that can also have unexpected conseqences

I agree, it's good to know that kind of magic but it's not necesary to use it often...

but I'm fine merging this and making it a major release after the changes for #131 are removed

Changes for issue #131 reverded.

[kpdecker]

Looks like even with the revert, there is still coverage missing. Could you make the tests green?

[piranna]

Looks like even with the revert, there is still coverage missing. Could you make the tests green?

Done, I wanted to check validity of the pull-request before.

[piranna]

Now tests coverage is passing on CI server

[kpdecker]

Released in 3.0.0

[piranna]

Thank you :-)

El 23/8/2016 14:17, "Kevin Decker" [email protected] escribió:

Released in 3.0.0

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#130 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAgfvlWbwEr-iiLEYpF8zWD85R9ty7aCks5qiuTkgaJpZM4Jn7pF
.