-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
.use(plugin) | ||
.run (err) -> | ||
if err | ||
resolve 'Cannot compress file: %s', err |
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.
why do you resolve on error? Why not reject? Same below.
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 will change it to reject with the same message, and use Cannot compress file:
as a constance.
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 would not use same error description / constant. It would be useful to know what caused an error. So here i would maybe write Cannot compress file: %s
but below Cannot get stats for source: %s
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.
But personally i always try to use word Error
on error. It allows easier searching through log files for potential issues.
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.
Ok, thanks!
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.
@butenkor I've updated according to your comments. Please check. |
@@ -26,11 +26,11 @@ class Compress | |||
.use(plugin) | |||
.run (err) -> | |||
if err | |||
resolve 'Cannot compress file: %s', err | |||
reject 'error while compressing file: ' + err |
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.
Please always start sentence with upper case. Search for most tools is case insensitive
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 good. Just check my last two comments and you are good to merge. |
👍 for coverage increase by 23% :) |
👍 |
WIP @panshul007, could you have a look?