Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added possibility to set binary path of pngquant manually #10

Merged
merged 5 commits into from
Jul 19, 2016

Conversation

fela98
Copy link
Contributor

@fela98 fela98 commented Jul 19, 2016

Added possibility to set binary path of pngquant manually. Pretty useful when using this tool and if you want to hijack the command line with for instance a Docker container. This would also make it possible to use a specific version (or fork) of pngquant on a system where multiple versions are installed.

@coveralls
Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage decreased (-2.4%) to 85.0% when pulling 29dba17 on fela98:master into e55a918 on papandreou:master.

@@ -22,6 +22,11 @@ function PngQuant(pngQuantArgs) {
util.inherits(PngQuant, Stream);

PngQuant.getBinaryPath = memoizeAsync(function (cb) {
if(this.binaryPath !== undefined) {
cb(this.binaryPath);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to be releasing Zalgo here by calling the callback immediately. Also, it should be cb(null, this.binaryPath); so this.binaryPath isn't interpreted as an error.

@papandreou
Copy link
Owner

Thanks for the contribution! I'm cool with the feature, but have a quibble with the implementation -- se my comment :)

@fela98
Copy link
Contributor Author

fela98 commented Jul 19, 2016

Sorry for the mistakes, hope I fixed them now. Is this ok to merge now or are there still things that need a fix?

@papandreou
Copy link
Owner

👍, merging.

@papandreou papandreou merged commit b35a14b into papandreou:master Jul 19, 2016
@papandreou
Copy link
Owner

papandreou commented Jul 19, 2016

Released as 1.1.0, thanks!

@coveralls
Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage decreased (-3.2%) to 84.158% when pulling 472bb84 on fela98:master into e55a918 on papandreou:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants