util: refactor createZip#190
util: refactor createZip#190ctalkington wants to merge 1 commit intojquery:masterfrom ctalkington:zipTo
Conversation
|
@rxaviers mind giving this a test? |
lib/util.js
Outdated
There was a problem hiding this comment.
I guess this is the only use of async in this file. We should also remove the var async... in the beginning of the file.
There was a problem hiding this comment.
yah, i had noticed that. i can adjust before merge.
|
Thank you for your contribution. Tests passed just fine. Before landing it, I need you to electronically sign http://contribute.jquery.org/CLA/, then I need you to review some comments I left. |
|
CLA should already be signed. |
Great! |
|
let me know if that is better. |
|
It's excellent. I will just wait on @scottgonzalez review. Then, I will ask you to squash commits into your first one, and update its title to: "Util: Refactor..." (notice both uppercase). |
|
It's worth saying I liked you are keeping commits separate during review though. |
lib/util.js
Outdated
There was a problem hiding this comment.
Use full words for variable names.
|
Just some minor style issues, but other than that looks good. |
|
ok, ive squashed and believe have made all requested changes. let me know if anything else needs adjusted. this will effect my other PR which i'll fix up over weekend. |
|
Thank you |
lib/util.js
Outdated
There was a problem hiding this comment.
One last comment. Could we rename s/event/finishEvent?
…ial loss of data with fs streams.
|
Closed by 9c7e277 |
to not need async wrapper and prevent potential loss of data with fs streams.