selaux / node-sprite-generator

Generates image sprites and their spritesheets (css, stylus, sass or less) from sets of images. Supports retina sprites. Provides express middleware and grunt task.
MIT License
192 stars 39 forks source link

Replace backslashes in relativePath by slashes for windows compatiblity #47

Closed mecab closed 8 years ago

mecab commented 8 years ago

Hi,

I found that options.spritePath contains backslashes when we use the module on Windows. This PR will solve the issue.

selaux commented 8 years ago

Thanks for the PR (and the included bug report :+1: ). To not be required to sniff the platform, we could rather use relativePath = relativePath.replace(path.sep, '/');.

What about escaped backslashes in the path, is that possible?

And for pull requests to be merged here they need to include tests :wink:.

mecab commented 8 years ago

Thanks for the response. Using path.sep is nice👍. I've updated it to use path.sep.

I'm stuck in writing test since we have to change path module's behavior to test the code. Should I have to write fake path module which use backslash as separator, regardless of actual platform?

Please let me know if you have any idea 💫.

mecab commented 8 years ago

Thank you very much for the advise. I think I have done the work.

While proxyquire is nice, I found the more specific code to test getRelativeSpriteDir() so I added proxyquired test for there.

I also added path module's mock for Node.js <= v0.10, which does not have path.win32. It might be poor a bit, but it does its job at least 👍

selaux commented 8 years ago

Damn node 0.10, making things so compicated :wink:.

mecab commented 8 years ago

Sorry for the late response, but shouldn't they be Windows paths then? 'test\img\sprite.png'

Oops, I forgot to include them. Now it's confirmed to work well in the latest commit, with adding more mocked functions😎.

selaux commented 8 years ago

Just asking: What is the status of this? Is this good to merge? To me it looks fine.

mecab commented 8 years ago

Yes, It's ready to merge, I think.