Add support for complex file extensions like ".coffee.md" or ".html.erb" #3122
Conversation
The entries in the fileNames field of a language definition can now also contain extensions
|
Open for community committers. |
| // E.g. "coffee.md", then "md", unless "coffee.md" resolves to a language | ||
| extension = parts.slice(i).join("."); | ||
| language = _fileExtensionToLanguageMap[extension]; | ||
| extensions.push(extension); |
There was a problem hiding this comment.
Would this logic cause a file named "html" (with no extension) to get resolved to HTML since we're treatig it like ".html"? Ideally, it seems like we should distinguish filenames from extensions more clearly...
There was a problem hiding this comment.
"html" would be a file name. The extension logic only kicks in when there's actually a dot present (due to i=1 instead of i=0).
There was a problem hiding this comment.
I've checked, and it works allright. It also works nicely with other filenames such as .bashrc or .profile, detecting first filename and then extension.
@peterflynn was your concern fully addressed, or is there something else?
There was a problem hiding this comment.
Hidden files are an interesting edge case... it probably would treat ".profile" first as a file name, and then use the extension "profile". I don't think that's what we want since the leading dot has different semantics. I will change the code to take this into account.
There was a problem hiding this comment.
Yes, that's exactly what happens. First it looks for .profile as filename and then for profile as extension. I thought that would work, but now that you mention it, I agree that we'd want it to work slightly different.
ghost
assigned 
|
Reviewing... |
|
There's a conflict while merging... mostly due to ba472a3 opening the API. Please, merge with master. |
| expect(LanguageManager.getLanguageForPath("foo.erb")).toBe(unknown); | ||
|
|
||
| html._addFileExtension("html.erb"); | ||
| ruby._addFileExtension("erb"); |
There was a problem hiding this comment.
_addFileExtension is already public in master
|
Finish reviewing... Looks good, and as we saw in #3083, it already solves a possible use case for ruby files. @DennisKehrig Please, merge with master. While you're at it, could you update the jsdocs descriptions for |
Conflicts: src/language/LanguageManager.js
…oesn't have a file extension) Don't spam the console when no language could be found for an extension
Conflicts: src/thirdparty/CodeMirror2
|
Finally: all done! |
|
I take that back, a unit test fails. |
|
Good to go now. |
| parts = fileName.split("."); | ||
| extensions = []; | ||
| // For file name "foo.coffee.md", consider "coffee.md" and "md" as extensions | ||
| // Treat file name ".coffee.md" as a hidden file named "coffee.md" and only consider "md" as extension |
There was a problem hiding this comment.
This seems a little bit inconsistent. Above, fileName returned from PathUtils.filename for a file named .coffee.md includes the dot, and here it points to that it is only a marker for hidden files. This is enforced in that you need to use .coffe.md in languages.json as a filename and not coffee.md. I think the behavior is correct as is, so maybe we could just change the comment... what do you think?
There was a problem hiding this comment.
I see your point... I'll try to rephrase it better, so that it's clear that this is just the rationale for ignoring a leading dot.
|
@DennisKehrig Done with re-review. Behaviour looks great! Just a couple of minor comments... |
|
Also, it looks that one of the last merges has created a conflict here, so please, merge with master again :S |
|
Thank you for the review! I'll get to this tomorrow. |
|
Superseded by #3285, closing |
The entries in the fileNames field of a language definition can now also contain extensions since file names are checked before extensions are checked.