-
Notifications
You must be signed in to change notification settings - Fork 640
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
Force unix style path separators in precompile #761
Conversation
@@ -88,6 +88,8 @@ function precompile(input, opts) { | |||
for(var i=0; i<templates.length; i++) { | |||
var name = templates[i].replace(path.join(input, '/'), ''); | |||
|
|||
name = name.replace('\\', '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a regex, this only replaces the first slash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a test that would have caught this.
Thanks for pointing that out, @rhengles. Sorted. @carljm, I gave some thought to a test. Given that the issue is platform specific, is it acceptable that the test would only ever fail on Windows? I was thinking of writing a test that ran a precompile, then checked the |
Yes, that's the sort of test that I was thinking of too. And yes, I think it's ok if it only fails on Windows. I guess we could write a test that would fail anywhere by mocking out some of the underlying path module? But that's harder, and I'm not convinced it's actually more useful. Better for the test to fail where the code would actually fail. We do have Windows CI. Thanks! |
IMHO, if we are standardizing on forward slashes, the test should simply disallow backwards slashes. If I understand this correctly. |
…ive of precompiling platform
Is there anything else I can do to progress this one? |
* Force unix style path seperators in precompile * Switched path sep replace, to use Regex * Added test to confirm that path seperators are always *NIX, irrespective of precompiling platform
I initially created mozilla#761 to deal with incorrectly formatted paths, when precompiling in Windows. The original pull request only dealt with directories, this change also deals with file names.
This change caused issues to our projects since we used to write paths in windows style, we had to force an older version of nunjucks as a sub dependecy of gulp-nunjucks by using npm shrinkwrap and forcing version 2.4.2 of nunjucks.
|
If you precompile in Windows, Nunjucks uses Windows path separators. This leads to inconsistent results, when precompiling between *NIX and Windows.
PR standardises path separators to *NIX style. All tests passing.