search

gemini-testing / looks-same

Node.js library for comparing images
MIT License
669 stars 55 forks source link

Added method createBuffer #6

Closed flore77 closed 9 years ago

flore77 commented 9 years ago

I have added the createBuffer method that we have spoke about. please take a look @SevInf. thanks

fixes #5

LinusU commented 9 years ago

You could use concat-stream instead of doing it yourself in the createBuffer method.

How about dropping the save: false and let it return a buffer when no diff: path is specified?

SevInf commented 9 years ago

Can you please squash the commits so it'll be either 1 commit, or 1 commit for code and 1 commit for tests?

How about dropping the save: false and let it return a buffer when no diff: path is specified?

@LinusU I like this idea.

flore77 commented 9 years ago

Can you please squash the commits

Yes, of course, I will squash the commits: 1 for the code and 1 for the tests.

dropping the save: false

I also like the idea to renounce to the save option

You could use concat-stream

I have thought that it is a simple function and I could write it myself, without adding another dependency. But I will use the concat-stream module.

I will do the changes. thank you both for your review @LinusU @SevInf

LinusU commented 9 years ago

I understand that feeling about modules, I can absolutely recommend reading Sindre Sorhus AMA answer on the topic, it's really good.

It's kind of funny actually because I used to write that exact snippet in many modules :smiley: made it shorter and shorter by using png.on('data', chunks.push.bind(chunks)) and such. But the reality is that it's very cheap to include another module and there will always be an edge case that someone else can fix; that fix will then propagate back to you.

Looks very good otherwise :+1:

flore77 commented 9 years ago

@SevInf Finally, I decided to rebase all commits in one. @LinusU Nice article :+1: And you are right, that a fix will propagate back to you, this is the beautiful of open source, that anyone can fix a bug and all dependents will benefit.

please review :smiley:

LinusU commented 9 years ago

Looks very good :+1:

SevInf commented 9 years ago

Thanks, looks great. Will publish a version in a minute.

SevInf commented 9 years ago

Published as 2.1.0. Thanks @flore77 for the code and @LinusU for the help with review.

LinusU commented 9 years ago

Thanks @SevInf for following semver to the letter, that's whats making the node ecosystem works! Great work with the module :+1: