Clean up _getFileExtension() method and fix all its callers #4846
Conversation
ghost
assigned 
| @@ -402,7 +390,7 @@ define(function (require, exports, module) { | |||
| */ | |||
| function _getFilenameWithoutExtension(filename) { | |||
| var extension = getFilenameExtension(filename); | |||
| return extension ? filename.replace(new RegExp(extension + "$"), "") : filename; | |||
| return extension ? filename.replace(new RegExp("." + extension + "$"), "") : filename; | |||
There was a problem hiding this comment.
I think it would more performant to just rewrite this as:
return filename.slice(0, filename.getLastIndexOf('.'));
|
Done with initial review. |
|
This PR has a jslint warning, since is now using |
| @@ -402,7 +390,7 @@ define(function (require, exports, module) { | |||
| */ | |||
| function _getFilenameWithoutExtension(filename) { | |||
| var extension = getFilenameExtension(filename); | |||
There was a problem hiding this comment.
This line isn't necessary.
|
You have a lint error on line 325. Move |
| * @private | ||
| * Get the file name without the extension. | ||
| * @param {string} filename File name of a file or directory | ||
| * @return {string} Returns the file name without the extension | ||
| */ | ||
| function _getFilenameWithoutExtension(filename) { | ||
| var extension = getFilenameExtension(filename); | ||
| return extension ? filename.replace(new RegExp(extension + "$"), "") : filename; | ||
| return filename.slice(0, filename.lastIndexOf('.')); |
There was a problem hiding this comment.
I think this breaks if the filename has no extension -- slice(0, -1) removes the last char.
There was a problem hiding this comment.
I tested this and it was fine. slice (0, -1) will basically return a new string that starts with 0 and has the length of the string
There was a problem hiding this comment.
Testing this again...
There was a problem hiding this comment.
slice(0, -1) will remove the last char according to the JavaScript docs.
There was a problem hiding this comment.
yeah, @peterflynn is right. You could pass undefined to slice if lastIndexOf() returned -1 e.g.
var index = lastIndexOf(".");
return filename.slice(0, index === -1 ? undefined : index);
There was a problem hiding this comment.
Or return index === -1 ? filename : filename.slice(0, index);
There was a problem hiding this comment.
Oh, I've missed slice(0, -1). What do you think about movin regexp+replace back? I think this func would be more obvious then.
There was a problem hiding this comment.
i'm not sure I understand your meaning of this func being more obvious. I think the regex adds a lot of overhead and is very slow compared to this method. This is far easier to read than regex, imo.
There was a problem hiding this comment.
Agreed -- a simple indexOf+slice seems cleaner to me too.
|
Closing in lieu of #5224 |
Fixes #4575
I've taken into account comments on this pull request and fixed it.