-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Correctly selects filename when known extension with a dot inside is used #7242
Conversation
var indexOfExtension = escapedName.lastIndexOf('.'); | ||
|
||
var indexOfExtension = escapedName.lastIndexOf("."), | ||
language = LanguageManager.getLanguageForPath(escapedName); |
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.
I don't think we should use escapedName
here - it could lead to issues. Better use entry.name
again.
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.
But the escaped one will be shown in the input so that's the one you need to be looking for indexOf, not?
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.
Ah, I get it, use original just for the language.
@zaggino This looks good, but it does not fix a case I was expecting. This works for a file named test.html.erb, but not test.php.js. That's because the file extension "html.erb" is explicity defined in languages.json, but "php.js" is not. "html.erb" is explicitly defined because it has to be -- it doesn't fit the accepted convention -- the files should be named test.erb.html. We don't want to have to list every combination of "php.*" (and other server-side markup languages) in languages.json -- we should automatically handle them. I think this can be done, without getting tricked by file names such as jquery-1.10.2.min.js, by also trying to match each known file extension part. For the test.php.js case, working backwards, it would check "js", then "php". Of course, it would also have to check "php.js" to handle the "html.erb" case. For the jquery-1.10.2.min.js case it would see "js" is known, but stop searching when it hits "min". Does that make sense? Care to give it a try? |
Sure, I want to finish this.
|
I think allowing any number of extensions in any combination would be most flexible. |
Review again please @redmunds - added also tests for things mentioned above. |
This works great! One more thing I notice is that the project tree highlighting still only recognizes the file extension the old way while file renaming recognizes file extension the new way: The project tree highlighting is done in |
Looks great! But, I'm seeing this JSLint unit test fail in your branch, but not in master: "should default to the editor's indent"
Seems like this is more related to your previous pull request. Are you seeing that failure? |
Oh, it's because your change to JSLint has not been merged into this branch. Looks good. Merging. |
Correctly selects filename when known extension with a dot inside is used
Follow up for #7209 in case we wanted to get fancy :-)
This is for #7265