uber-archive / image-diff

Create image differential between two images
MIT License
2.46k stars 94 forks source link

JSHint options cleanup + minor lint fixes #19

Closed XhmikosR closed 9 years ago

twolfson commented 9 years ago

Can you please provide more explanation for the introduction of these changes? To me, I see mostly noise of different development styles and would prefer more context.

XhmikosR commented 9 years ago

I honestly don't see where you see style changes.

But, anyway, mocha is a JSHint option which adds the needed predefined variables; it wasn't present in the ancient version currently used, hence the accompanied JSHint version update.

As for path.join it's the only cross-platform way to do things like that in node:

__dirname + '/actual-files':  C:\Users\xmr\Desktop\image-diff\test/actual-files
path.join(__dirname, '/actual-files'):  C:\Users\xmr\Desktop\image-diff\test\actual-files
twolfson commented 9 years ago

Ah, I see what's going on now.

Regarding the /* mocha: true */ option, I prefer to keep all the files running under the same lint conditions; one-off settings on a per-file basis makes the project feel inconsistent (because it is).

For fs interactions, node automatically coerces / to \ for Windows (similar to how it's doing it for the strings that include / in the path.join when they should be different parameters).

// Should really need to move to
path.join(__dirname, 'test-files', 'checkerboard.png');
// not
path.join(__dirname, '/test-files/checkerboard.png');

https://github.com/joyent/node/blob/v0.12.2/lib/fs.js#L482-L491

https://github.com/joyent/node/blob/v0.12.2/lib/path.js#L305-L327

https://github.com/joyent/node/blob/v0.12.2/lib/path.js#L531-L533

I am fine with upgrading mocha for the sake of more up to date dependencies but I think the other changes aren't a problem and causing PR noise.

XhmikosR commented 9 years ago

It's not the same conditions, though. You are polluting all the files with useless globals when they are only needed in one test file...

Anyway, I disagree with your whole PR noise; I find these changes consistent and cleaner, so closing this.