Skip to content
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

Fix 'ext' option for folders containing dot in name #625

Closed

Conversation

chrisirhc
Copy link

Problem

grunt.file.expandMapping doesn't work as expected when it encounters a folder with a dot in its name.

Current result:

expand/dee.p/dee.p.txt -> dest/expand/dee.foo

Expected result:

expand/dee.p/dee.p.txt -> dest/expand/dee.p/dee.p.foo

Proposed solution

Use last occurrence of '.' to replace the extension.

Tests fail because a dot is encountered before reaching the file when
specifying the 'ext' option.
Alternative method to replace the extension. Not sure if this method is
buggy is past versions of node.
@cowboy
Copy link
Member

cowboy commented Jan 21, 2013

There's two ways ext could work; it could consider everything after the first dot the extension, or everything after the last dot the extension—it can't be smart enough to "guess" automatically. We chose the former because the use-case is more common (we encounter .min.js files all the time). That being said, you can use the rename option to specify a function that will use whatever custom naming logic you need.

@cowboy cowboy closed this Jan 21, 2013
@chrisirhc
Copy link
Author

@cowboy I am not trying to say that we must chose the last dot, but my issue here is to replace the extension of the filename. Right now it replaces from the first dot in the path, which may be a parent/ancestor folder.
Maybe my example was not clear but if there is a folder containing an '.' in its name, it will replace that folder's extension instead of the file's extension.

For example mapping */.txt to dest/ with ext set to ".bar":

Source: b.ar/foo.txt
Dest: dest/b.bar

Instead of:

Source: b.ar/foo.txt
Dest: dest/b.ar/foo.bar

This can have unexpected consequences when mixing the ext option with and without flatten and/or cwd because the path has been modified instead of just the filename.

@cowboy
Copy link
Member

cowboy commented Jan 21, 2013

Ah, this is a good point. I'll try to fix this for 0.4.0, but it might have to wait for 0.4.1.

@cowboy cowboy reopened this Jan 21, 2013
@cowboy cowboy closed this in 2555495 Jan 21, 2013
@cowboy
Copy link
Member

cowboy commented Jan 21, 2013

Can you please clone grunt and test this to see if it fixes the issue?

@cowboy
Copy link
Member

cowboy commented Jan 21, 2013

(This is not published to npm yet, but will be as part of rc7 if there are no outstanding issues)

@chrisirhc
Copy link
Author

One other thing, it should make use of path.sep since the file path has not yet been normalized at this point.

On file path normalization, it file paths doesn't have to be done if path.sep === '/'.

Or you could use:

  destPath = destPath.replace(/(\.[^\/\\]*)?$/, options.ext);

@chrisirhc
Copy link
Author

Cloned and working on my Mac. Probably won't work on Windows.

@cowboy
Copy link
Member

cowboy commented Jan 21, 2013

Since the result from file.expand is being used, all paths should already be normalized to unix-style / separators at that point.

@chrisirhc
Copy link
Author

Ah, okay! Thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants