Fix tests, refactor some code

[saveman71]

Hello,

So I needed your module, and I thank you for your work. When I saw the tests were broken, I couldn't help but to look at the code. Well, I may have done too much, but I let you decide on that.

If you have some remarks about my coding style, please let me know! If you want me to revert the change of location of the files, let me know as well!

List of changes:

• Moved both files index.js and test.js to their respective directories.
• Re-factored tests to use the same a function testing both stream and buffer modes
• Styling improvements for index.js
• Updated dependencies
• Improved the gulpfile, replaced deprecated code

Cheers,

Antoine

[ben-eb]

Thanks for the pull request.

I think that this patch is a very large chunk to review as a whole; there are some elements I like here, some I don't. I'm not really sure that I would merge this as it stands, as the changes are not focused enough on a specific area, and instead have recommendations for test libraries that I don't really have an intention of using, and code that isn't relevant to the issue that you're solving (fixing tests!)

Especially in the case of should which extends the objects that you are testing, I think it's a kind of a bad practice - much prefer the expect style as I started with jasmine for running unit tests and it's what I'm familiar with. I would rather keep the chai library here for consistency across projects - all of my modules are re-using similar code, so it makes more sense for me when I have to update this.

The JSHint config is a little too much. It seems to be defining all of the relaxed options when it just doesn't need to. I think you can make a case for setting the node key, but I think having the predef array isn't as clear as using the code comments, because it forces you to think about the globals in that file. For example in the tests we would have describe but not when running the main library code. So to define describe globally doesn't really make sense to me. Plus setting relaxed options to false when by default they are false - just bloats the file and isn't necessary.

I'm not entirely sure why index.js needs to be moved into a subfolder; this is not a complex module. I would rather keep it simple, in one folder, where you don't have to define the main entry point (as it is index.js by default). I'm OK with moving the tests, but in this case I would argue that the fixture SVG should be extracted out of the file, if the goal is to simplify?

I think I would have rather that you had kept the existing code style - personal taste, I know, but I always preferred this way of writing if statements:

if (condition) {
   // ...
}

I think the rest looks good. I am aware of the updates to the gulp API and that running tasks inside gulp tasks isn't good practice. In fact I have been working on projects since that follow current best practice, just that I haven't had chance to update these gulpfiles just yet. So thanks for that. :smile:

OK so to sum;

• Thanks for your work! :+1:
• Changes to the functionality of index.js and the gulpfile.js are welcome improvements. :smile:
• No need to tweak the .jshintrc, just use the inline comments instead. :confused:
• Test location change is fine, if that also means that the fixtures are extracted out of the file. Otherwise just leave as is. :+1:
• Don't replace chai with should (and probably a good practice to follow for future pull requests -- don't replace modules just because you can!) :smiley:
• Try and stick to the original coding style where possible.

I think it might be more appropriate to open a new pull request with a squashed history that applies those changes, if you could. Thank you.

[saveman71]

Thanks for your reply!

I sure will modify my code to match your requirements!

• I will adapt my changes to your coding style
• I will create separate pull-requests as demanded
• No changes to the .jshintrc, noted

I created directories while thinking that it would help to identify key files in the repo, but I see on your others projects that it's not a practice of yours, so I'll stick to that.

And sorry for the should thing, it was a little rude, in retrospect :pensive: