Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Add support for complex file extensions like ".coffee.md" or ".html.erb" #3122

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 34 additions & 25 deletions src/language/LanguageManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@
* console.log("Language " + language.getName() + " is now available!");
* });
*
* The extension can also contain dots:
* LanguageManager.defineLanguage("literatecoffeescript", {
* name: "Literate CoffeeScript",
* mode: "coffeescript",
* fileExtensions: ["litcoffee", "coffee.md"]
* });
*
* You can also specify file names:
* LanguageManager.defineLanguage("makefile", {
* name: "Make",
Expand Down Expand Up @@ -133,21 +140,6 @@ define(function (require, exports, module) {
}
}

/**
* Lowercases the file extension and ensures it doesn't start with a dot.
* @param {!string} extension The file extension
* @return {string} The normalized file extension
*/
function _normalizeFileExtension(extension) {
// Remove a leading dot if present
if (extension.charAt(0) === ".") {
extension = extension.substr(1);
}

// Make checks below case-INsensitive
return extension.toLowerCase();
}

/**
* Monkey-patch CodeMirror to prevent modes from being overwritten by extensions.
* We may rely on the tokens provided by some of these modes.
Expand Down Expand Up @@ -188,16 +180,31 @@ define(function (require, exports, module) {

/**
* Resolves a file path to a Language object.
* File names have a higher priority than file extensions.
* @param {!string} path Path to the file to find a language for
* @return {Language} The language for the provided file type or the fallback language
*/
function getLanguageForPath(path) {
var extension = _normalizeFileExtension(PathUtils.filenameExtension(path)),
filename = PathUtils.filename(path).toLowerCase(),
language = extension ? _fileExtensionToLanguageMap[extension] : _fileNameToLanguageMap[filename];
var fileName = PathUtils.filename(path).toLowerCase(),
language = _fileNameToLanguageMap[fileName],
extension,
extensions,
parts,
i,
l;

if (!language) {
console.log("Called LanguageManager.getLanguageForPath with an unhandled " + (extension ? "file extension" : "file name") + ":", extension || filename);
// Split "foo.coffee.md" into ["foo", "coffee", "md"]
// Split ".profile" into ["", "profile"]
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (i = parts[0] === "" ? 2 : 1, l = parts.length; i < l && !language; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As cool as this oneliner is, I find it a little bit hard to read. Even more now that we've added the condition inside the declarations part. How would you feel about extracting the declarations and using a while loop instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely seems warranted :)

extension = parts.slice(i).join(".");
language = _fileExtensionToLanguageMap[extension];
extensions.push(extension);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

return language || _fallbackLanguage;
Expand Down Expand Up @@ -400,12 +407,16 @@ define(function (require, exports, module) {

/**
* Adds a file extension to this language.
* Private for now since dependent code would need to by kept in sync with such changes.
* See https://github.com/adobe/brackets/issues/2966 for plans to make this public.
* @param {!string} extension A file extension used by this language
*/
Language.prototype.addFileExtension = function (extension) {
extension = _normalizeFileExtension(extension);
// Remove a leading dot if present
if (extension.charAt(0) === ".") {
extension = extension.substr(1);
}

// Make checks below case-INsensitive
extension = extension.toLowerCase();

if (this._fileExtensions.indexOf(extension) === -1) {
this._fileExtensions.push(extension);
Expand All @@ -423,12 +434,10 @@ define(function (require, exports, module) {

/**
* Adds a file name to the language which is used to match files that don't have extensions like "Makefile" for example.
* Private for now since dependent code would need to by kept in sync with such changes.
* See https://github.com/adobe/brackets/issues/2966 for plans to make this public.
* @param {!string} extension An extensionless file name used by this language
* @private
*/
Language.prototype.addFileName = function (name) {
// Make checks below case-INsensitive
name = name.toLowerCase();

if (this._fileNames.indexOf(name) === -1) {
Expand Down
15 changes: 15 additions & 0 deletions test/spec/LanguageManager-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,21 @@ define(function (require, exports, module) {
expect(LanguageManager.getLanguageForPath("foo.doesNotExist")).toBe(unknown);
});

it("should map complex file extensions to languages", function () {
var ruby = LanguageManager.getLanguage("ruby"),
html = LanguageManager.getLanguage("html"),
unknown = LanguageManager.getLanguage("unknown");

expect(LanguageManager.getLanguageForPath("foo.html.erb")).toBe(unknown);
expect(LanguageManager.getLanguageForPath("foo.erb")).toBe(unknown);

html.addFileExtension("html.erb");
ruby.addFileExtension("erb");

expect(LanguageManager.getLanguageForPath("foo.html.erb")).toBe(html);
expect(LanguageManager.getLanguageForPath("foo.erb")).toBe(ruby);
});

it("should map file names to languages", function () {
var coffee = LanguageManager.getLanguage("coffeescript"),
unknown = LanguageManager.getLanguage("unknown");
Expand Down