From d71b7b865b6fbdcc2f50ae9621864173735376b0 Mon Sep 17 00:00:00 2001 From: Dennis Kehrig Date: Mon, 4 Mar 2013 15:35:09 +0100 Subject: [PATCH 1/6] Fixed outdated examples of using the LanguageManager --- src/language/LanguageManager.js | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index 04e8c045ac5..4bdfd818652 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -64,7 +64,7 @@ * To find existing MIME modes, search for "CodeMirror.defineMIME" in thirdparty/CodeMirror2/mode * For instance, C++, C# and Java all use the clike (C-like) mode with different settings and a different MIME name. * You can refine the mode definition by specifying the MIME mode as well: - * var language = LanguageManager.defineLanguage("csharp", { + * LanguageManager.defineLanguage("csharp", { * name: "C#", * mode: ["clike", "text/x-csharp"], * ... @@ -80,7 +80,7 @@ * "mode": null}] * }); * - * var language = LanguageManager.defineLanguage("html", { + * LanguageManager.defineLanguage("html", { * name: "HTML", * mode: ["htmlmixed", "text/x-brackets-html"], * ... @@ -110,24 +110,15 @@ define(function (require, exports, module) { // Helper functions /** - * Checks whether value is a string. Throws an exception otherwise. + * Checks whether value is a non-empty string. Throws an exception otherwise. * @param {*} value The value to validate * @param {!string} description A helpful identifier for value */ - function _validateString(value, description) { + function _validateNonEmptyString(value, description) { // http://stackoverflow.com/questions/1303646/check-whether-variable-is-number-or-string-in-javascript if (Object.prototype.toString.call(value) !== "[object String]") { throw new Error(description + " must be a string"); } - } - - /** - * Checks whether value is a non-empty string. Throws an exception otherwise. - * @param {*} value The value to validate - * @param {!string} description A helpful identifier for value - */ - function _validateNonEmptyString(value, description) { - _validateString(value, description); if (value === "") { throw new Error(description + " must not be empty"); } @@ -248,7 +239,7 @@ define(function (require, exports, module) { * @param {!string} name Human-readable name of the language, as it's commonly referred to (i.e. "C++") */ function Language(id, name) { - _validateString(id, "Language ID"); + _validateNonEmptyString(id, "Language ID"); // Make sure the ID is a string that can safely be used universally by the computer - as a file name, as an object key, as part of a URL, etc. // Hence we use "_" instead of "." since the latter often has special meaning if (!id.match(/^[a-z0-9]+(_[a-z0-9]+)*$/)) { From 796071bf65f3bebd06cdb9d372f1eb538bee626c Mon Sep 17 00:00:00 2001 From: Dennis Kehrig Date: Mon, 4 Mar 2013 17:17:54 +0100 Subject: [PATCH 2/6] Fixes #2967: LanguageManager shouldn't throw exceptions --- src/language/LanguageManager.js | 160 +++++++++++++++++++----------- test/spec/LanguageManager-test.js | 45 ++++++--- 2 files changed, 135 insertions(+), 70 deletions(-) diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index 4bdfd818652..5651ce09c34 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -110,18 +110,27 @@ define(function (require, exports, module) { // Helper functions /** - * Checks whether value is a non-empty string. Throws an exception otherwise. - * @param {*} value The value to validate - * @param {!string} description A helpful identifier for value + * Checks whether value is a non-empty string. Reports an error otherwise. + * If no deferred is passed, console.error is called. + * Otherwise the deferred is rejected with the error message. + * @param {*} value The value to validate + * @param {!string} description A helpful identifier for value + * @param {?jQuery.Deferred} deferred A deferred to reject with the error message in case of an error + * @return {boolean} True if the value is a non-empty string, false otherwise */ - function _validateNonEmptyString(value, description) { + function _validateNonEmptyString(value, description, deferred) { + var reportError = deferred ? deferred.reject : console.error; + // http://stackoverflow.com/questions/1303646/check-whether-variable-is-number-or-string-in-javascript if (Object.prototype.toString.call(value) !== "[object String]") { - throw new Error(description + " must be a string"); + reportError(description + " must be a string"); + return false; } if (value === "") { - throw new Error(description + " must not be empty"); + reportError(description + " must not be empty"); + return false; } + return true; } /** @@ -147,7 +156,8 @@ define(function (require, exports, module) { var _original_CodeMirror_defineMode = CodeMirror.defineMode; function _wrapped_CodeMirror_defineMode(name) { if (CodeMirror.modes[name]) { - throw new Error("There already is a CodeMirror mode with the name \"" + name + "\""); + console.error("There already is a CodeMirror mode with the name \"" + name + "\""); + return; } _original_CodeMirror_defineMode.apply(CodeMirror, arguments); } @@ -234,26 +244,8 @@ define(function (require, exports, module) { /** * @constructor * Model for a language. - * - * @param {!string} id Identifier for this language, use only letters a-z and _ inbetween (i.e. "cpp", "foo_bar") - * @param {!string} name Human-readable name of the language, as it's commonly referred to (i.e. "C++") */ - function Language(id, name) { - _validateNonEmptyString(id, "Language ID"); - // Make sure the ID is a string that can safely be used universally by the computer - as a file name, as an object key, as part of a URL, etc. - // Hence we use "_" instead of "." since the latter often has special meaning - if (!id.match(/^[a-z0-9]+(_[a-z0-9]+)*$/)) { - throw new Error("Invalid language ID \"" + id + "\": Only groups of lower case letters and numbers are allowed, separated by underscores."); - } - if (_languages[id]) { - throw new Error("Language \"" + id + "\" is already defined"); - } - - _validateNonEmptyString(name, "name"); - - this._id = id; - this._name = name; - + function Language() { this._fileExtensions = []; this._fileNames = []; this._modeToLanguageMap = {}; @@ -292,6 +284,26 @@ define(function (require, exports, module) { Language.prototype.getId = function () { return this._id; }; + + /** + * Sets the identifier for this language or prints an error to the console. + * @param {!string} id Identifier for this language, use only letters a-z and _ inbetween (i.e. "cpp", "foo_bar") + * @return {boolean} Whether the ID was valid and set or not + */ + Language.prototype._setId = function (id) { + if (!_validateNonEmptyString(id, "Language ID")) { + return false; + } + // Make sure the ID is a string that can safely be used universally by the computer - as a file name, as an object key, as part of a URL, etc. + // Hence we use "_" instead of "." since the latter often has special meaning + if (!id.match(/^[a-z0-9]+(_[a-z0-9]+)*$/)) { + console.error("Invalid language ID \"" + id + "\": Only groups of lower case letters and numbers are allowed, separated by underscores."); + return false; + } + + this._id = id; + return true; + }; /** * Returns the human-readable name of this language. @@ -301,6 +313,20 @@ define(function (require, exports, module) { return this._name; }; + /** + * Sets the human-readable name of this language or prints an error to the console. + * @param {!string} name Human-readable name of the language, as it's commonly referred to (i.e. "C++") + * @return {boolean} Whether the name was valid and set or not + */ + Language.prototype._setName = function (name) { + if (!_validateNonEmptyString(name, "name")) { + return false; + } + + this._name = name; + return true; + }; + /** * Returns the CodeMirror mode for this language. * @return {string} The mode @@ -324,19 +350,23 @@ define(function (require, exports, module) { if (Array.isArray(mode)) { if (mode.length !== 2) { - throw new Error("Mode must either be a string or an array containing two strings"); + result.reject("Mode must either be a string or an array containing two strings"); + return result.promise(); } mimeMode = mode[1]; mode = mode[0]; } // mode must not be empty. Use "null" (the string "null") mode for plain text - _validateNonEmptyString(mode, "mode"); + if (!_validateNonEmptyString(mode, "mode", result)) { + result.reject(); + return result.promise(); + } var finish = function () { if (!CodeMirror.modes[mode]) { result.reject("CodeMirror mode \"" + mode + "\" is not loaded"); - return; + return result.promise(); } if (mimeMode) { @@ -344,13 +374,13 @@ define(function (require, exports, module) { if (!modeConfig) { result.reject("CodeMirror MIME mode \"" + mimeMode + "\" not found"); - return; + return result.promise(); } // modeConfig can be a string or mode object if (modeConfig !== mode && modeConfig.name !== mode) { result.reject("CodeMirror MIME mode \"" + mimeMode + "\" does not belong to mode \"" + mode + "\""); - return; + return result.promise(); } } @@ -391,13 +421,11 @@ 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 + * @return {boolean} Whether adding the file extension was successful or not */ Language.prototype.addFileExtension = function (extension) { extension = _normalizeFileExtension(extension); - if (this._fileExtensions.indexOf(extension) === -1) { this._fileExtensions.push(extension); @@ -414,10 +442,7 @@ 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) { name = name.toLowerCase(); @@ -434,6 +459,7 @@ define(function (require, exports, module) { this._wasModified(); } + return true; }; /** @@ -453,9 +479,10 @@ define(function (require, exports, module) { }; /** - * Sets the prefixes to use for line comments in this language. + * Sets the prefixes to use for line comments in this language or prints an error to the console. * @param {!string|Array.} prefix Prefix string or and array of prefix strings * to use for line comments (i.e. "//" or ["//", "#"]) + * @return {boolean} Whether the syntax was valid and set or not */ Language.prototype.setLineCommentSyntax = function (prefix) { var prefixes = Array.isArray(prefix) ? prefix : [prefix]; @@ -472,6 +499,8 @@ define(function (require, exports, module) { } else { console.error("The prefix array should not be empty"); } + + return true; }; /** @@ -499,16 +528,20 @@ define(function (require, exports, module) { }; /** - * Sets the prefix and suffix to use for blocks comments in this language. + * Sets the prefix and suffix to use for blocks comments in this language or prints an error to the console. * @param {!string} prefix Prefix string to use for block comments (e.g. "") + * @return {boolean} Whether the syntax was valid and set or not */ Language.prototype.setBlockCommentSyntax = function (prefix, suffix) { - _validateNonEmptyString(prefix, "prefix"); - _validateNonEmptyString(suffix, "suffix"); + if (!_validateNonEmptyString(prefix, "prefix") || !_validateNonEmptyString(suffix, "suffix")) { + return false; + } this._blockCommentSyntax = { prefix: prefix, suffix: suffix }; this._wasModified(); + + return true; }; /** @@ -521,23 +554,27 @@ define(function (require, exports, module) { if (mode === this._mode) { return this; } - return this._modeToLanguageMap[mode] || _getLanguageForMode(mode); }; /** - * Overrides a mode-to-language association for this particular language only. + * Overrides a mode-to-language association for this particular language only or prints an error to the console. * Used to disambiguate modes used by multiple languages. * @param {!string} mode The mode to associate the language with * @param {!Language} language The language to associate with the mode + * @return {boolean} Whether the mode-to-language association was valid and set or not * @private */ Language.prototype._setLanguageForMode = function (mode, language) { if (mode === this._mode && language !== this) { - throw new Error("A language must always map its mode to itself"); + console.error("A language must always map its mode to itself"); + return false; } + this._modeToLanguageMap[mode] = language; this._wasModified(); + + return true; }; /** @@ -580,21 +617,27 @@ define(function (require, exports, module) { result.reject("Language \"" + id + "\" is waiting to be resolved."); return result.promise(); } - - var language = new Language(id, definition.name), - fileExtensions = definition.fileExtensions, - fileNames = definition.fileNames, - l, - i; - - var blockComment = definition.blockComment; - if (blockComment) { - language.setBlockCommentSyntax(blockComment[0], blockComment[1]); + if (_languages[id]) { + console.error("Language \"" + id + "\" is already defined"); + return result.promise(); } + + var language = new Language(), + name = definition.name, + fileExtensions = definition.fileExtensions, + fileNames = definition.fileNames, + blockComment = definition.blockComment, + lineComment = definition.lineComment, + i, + l; - var lineComment = definition.lineComment; - if (lineComment) { - language.setLineCommentSyntax(lineComment); + if (!language._setId(id) + || !language._setName(name) + || (blockComment && !language.setBlockCommentSyntax(blockComment[0], blockComment[1])) + || (lineComment && !language.setLineCommentSyntax(lineComment)) + ) { + result.reject(); + return result.promise(); } // track languages that are currently loading @@ -618,6 +661,9 @@ define(function (require, exports, module) { // globally associate mode to language _setLanguageForMode(language.getMode(), language); + // finally, store language to _language map + _languages[language.getId()] = language; + // fire an event to notify DocumentManager of the new language _triggerLanguageAdded(language); diff --git a/test/spec/LanguageManager-test.js b/test/spec/LanguageManager-test.js index e81004b73b5..c3df3b30266 100644 --- a/test/spec/LanguageManager-test.js +++ b/test/spec/LanguageManager-test.js @@ -164,21 +164,34 @@ define(function (require, exports, module) { validateLanguage(def, language); }); - it("should throw errors for invalid language id values", function () { - expect(function () { defineLanguage({ id: null }); }).toThrow(new Error("Language ID must be a string")); - expect(function () { defineLanguage({ id: "HTML5" }); }).toThrow(new Error("Invalid language ID \"HTML5\": Only groups of lower case letters and numbers are allowed, separated by underscores.")); - expect(function () { defineLanguage({ id: "_underscore" }); }).toThrow(new Error("Invalid language ID \"_underscore\": Only groups of lower case letters and numbers are allowed, separated by underscores.")); - expect(function () { defineLanguage({ id: "html" }); }).toThrow(new Error('Language "html" is already defined')); + it("should print errors for invalid language id values", function () { + defineLanguage({ id: null }); + expect(console.error).toHaveBeenCalledWith("Language ID must be a string"); + + defineLanguage({ id: "HTML5" }); + expect(console.error).toHaveBeenCalledWith("Invalid language ID \"HTML5\": Only groups of lower case letters and numbers are allowed, separated by underscores."); + + defineLanguage({ id: "_underscore" }); + expect(console.error).toHaveBeenCalledWith("Invalid language ID \"_underscore\": Only groups of lower case letters and numbers are allowed, separated by underscores."); + + defineLanguage({ id: "html" }); + expect(console.error).toHaveBeenCalledWith("Language \"html\" is already defined"); }); it("should throw errors for invalid language name values", function () { - expect(function () { defineLanguage({ id: "two" }); }).toThrow(new Error("name must be a string")); - expect(function () { defineLanguage({ id: "three", name: "" }); }).toThrow(new Error("name must not be empty")); + defineLanguage({ id: "two" }); + expect(console.error).toHaveBeenCalledWith("name must be a string"); + + defineLanguage({ id: "three", name: "" }); + expect(console.error).toHaveBeenCalledWith("name must not be empty"); }); it("should log errors for missing mode value", function () { - expect(function () { defineLanguage({ id: "four", name: "Four" }); }).toThrow(new Error("mode must be a string")); - expect(function () { defineLanguage({ id: "five", name: "Five", mode: "" }); }).toThrow(new Error("mode must not be empty")); + defineLanguage({ id: "four", name: "Four" }); + expect(console.error).toHaveBeenCalledWith("mode must be a string"); + + defineLanguage({ id: "five", name: "Five", mode: "" }); + expect(console.error).toHaveBeenCalledWith("mode must not be empty"); }); it("should create a language with file extensions and a mode", function () { @@ -235,7 +248,8 @@ define(function (require, exports, module) { var def = { id: "pascal", name: "Pascal", fileExtensions: ["pas", "p"], mode: "pascal" }; runs(function () { - expect(function () { defineLanguage(def); }).toThrow(new Error('Language "pascal" is already defined')); + defineLanguage(def); + expect(console.error).toHaveBeenCalledWith("Language \"pascal\" is already defined"); }); }); @@ -254,9 +268,14 @@ define(function (require, exports, module) { }, "The language should be resolved", 50); runs(function () { - expect(function () { language.setLineCommentSyntax(""); }).toThrow(new Error("prefix must not be empty")); - expect(function () { language.setBlockCommentSyntax(""); }).toThrow(new Error("prefix must not be empty")); + language.setLineCommentSyntax(""); + expect(console.error).toHaveBeenCalledWith("prefix must not be empty"); + + language.setBlockCommentSyntax(""); + expect(console.error).toHaveBeenCalledWith("prefix must not be empty"); def.lineComment = "//"; def.blockComment = { From c06638a203c8ae75e2e1f1c40c2415322e7434f9 Mon Sep 17 00:00:00 2001 From: Dennis Kehrig Date: Wed, 13 Mar 2013 11:22:31 +0100 Subject: [PATCH 3/6] Addressed Jason's review concerns --- src/language/LanguageManager.js | 8 ++++---- test/spec/LanguageManager-test.js | 23 +++++++++++++++-------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index 5651ce09c34..634aabccc6d 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -366,7 +366,7 @@ define(function (require, exports, module) { var finish = function () { if (!CodeMirror.modes[mode]) { result.reject("CodeMirror mode \"" + mode + "\" is not loaded"); - return result.promise(); + return; } if (mimeMode) { @@ -374,13 +374,13 @@ define(function (require, exports, module) { if (!modeConfig) { result.reject("CodeMirror MIME mode \"" + mimeMode + "\" not found"); - return result.promise(); + return; } // modeConfig can be a string or mode object if (modeConfig !== mode && modeConfig.name !== mode) { result.reject("CodeMirror MIME mode \"" + mimeMode + "\" does not belong to mode \"" + mode + "\""); - return result.promise(); + return; } } @@ -618,7 +618,7 @@ define(function (require, exports, module) { return result.promise(); } if (_languages[id]) { - console.error("Language \"" + id + "\" is already defined"); + result.reject("Language \"" + id + "\" is already defined"); return result.promise(); } diff --git a/test/spec/LanguageManager-test.js b/test/spec/LanguageManager-test.js index c3df3b30266..86c002adb72 100644 --- a/test/spec/LanguageManager-test.js +++ b/test/spec/LanguageManager-test.js @@ -164,7 +164,7 @@ define(function (require, exports, module) { validateLanguage(def, language); }); - it("should print errors for invalid language id values", function () { + it("should log errors for invalid language id values", function () { defineLanguage({ id: null }); expect(console.error).toHaveBeenCalledWith("Language ID must be a string"); @@ -173,12 +173,9 @@ define(function (require, exports, module) { defineLanguage({ id: "_underscore" }); expect(console.error).toHaveBeenCalledWith("Invalid language ID \"_underscore\": Only groups of lower case letters and numbers are allowed, separated by underscores."); - - defineLanguage({ id: "html" }); - expect(console.error).toHaveBeenCalledWith("Language \"html\" is already defined"); }); - it("should throw errors for invalid language name values", function () { + it("should log errors for invalid language name values", function () { defineLanguage({ id: "two" }); expect(console.error).toHaveBeenCalledWith("name must be a string"); @@ -245,11 +242,21 @@ define(function (require, exports, module) { // FIXME: Add internal LanguageManager._reset() // or unload a language (pascal is loaded from the previous test) it("should return an error if a language is already defined", function () { - var def = { id: "pascal", name: "Pascal", fileExtensions: ["pas", "p"], mode: "pascal" }; + var def = { id: "pascal", name: "Pascal", fileExtensions: ["pas", "p"], mode: "pascal" }, + error = -1; + + runs(function () { + defineLanguage(def).fail(function (err) { + error = err; + }); + }); + + waitsFor(function () { + return error !== -1; + }, "The promise should be rejected with an error", 50); runs(function () { - defineLanguage(def); - expect(console.error).toHaveBeenCalledWith("Language \"pascal\" is already defined"); + expect(error).toBe("Language \"pascal\" is already defined"); }); }); From 56251498646b0ea6325bfd9191e677dfdc75de58 Mon Sep 17 00:00:00 2001 From: Dennis Kehrig Date: Wed, 13 Mar 2013 13:35:00 +0100 Subject: [PATCH 4/6] Fix a JSHint complaint --- src/language/LanguageManager.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index 634aabccc6d..a187cdd13e2 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -631,11 +631,9 @@ define(function (require, exports, module) { i, l; - if (!language._setId(id) - || !language._setName(name) - || (blockComment && !language.setBlockCommentSyntax(blockComment[0], blockComment[1])) - || (lineComment && !language.setLineCommentSyntax(lineComment)) - ) { + if (!language._setId(id) || !language._setName(name) || + (blockComment && !language.setBlockCommentSyntax(blockComment[0], blockComment[1])) || + (lineComment && !language.setLineCommentSyntax(lineComment))) { result.reject(); return result.promise(); } From 9267dac40be728ea1cde3d9b23053dbedd8ab25d Mon Sep 17 00:00:00 2001 From: Dennis Kehrig Date: Tue, 19 Mar 2013 16:56:40 +0100 Subject: [PATCH 5/6] Fixed unit tests --- src/language/LanguageManager.js | 2 +- test/spec/LanguageManager-test.js | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index a187cdd13e2..3bfbebc74bd 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -491,7 +491,7 @@ define(function (require, exports, module) { if (prefixes.length) { this._lineCommentSyntax = []; for (i = 0; i < prefixes.length; i++) { - _validateNonEmptyString(String(prefixes[i]), "prefix"); + _validateNonEmptyString(String(prefixes[i]), Array.isArray(prefix) ? "prefix[" + i + "]" : "prefix"); this._lineCommentSyntax.push(prefixes[i]); } diff --git a/test/spec/LanguageManager-test.js b/test/spec/LanguageManager-test.js index 86c002adb72..dcf3f1c3e2b 100644 --- a/test/spec/LanguageManager-test.js +++ b/test/spec/LanguageManager-test.js @@ -315,8 +315,11 @@ define(function (require, exports, module) { language.setLineCommentSyntax([]); expect(console.error).toHaveBeenCalledWith("The prefix array should not be empty"); - expect(function () { language.setLineCommentSyntax([""]); }).toThrow(new Error("prefix must not be empty")); - expect(function () { language.setLineCommentSyntax(["#", ""]); }).toThrow(new Error("prefix must not be empty")); + language.setLineCommentSyntax([""]); + expect(console.error).toHaveBeenCalledWith("prefix[0] must not be empty"); + + language.setLineCommentSyntax(["#", ""]); + expect(console.error).toHaveBeenCalledWith("prefix[1] must not be empty"); def.lineComment = ["#"]; From 9d36286f0f89d74d5be0372c61d2f1428b627982 Mon Sep 17 00:00:00 2001 From: Dennis Kehrig Date: Tue, 19 Mar 2013 16:58:52 +0100 Subject: [PATCH 6/6] Added a missing piece of documentation --- src/language/LanguageManager.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js index 3bfbebc74bd..8ec1f0caaa1 100644 --- a/src/language/LanguageManager.js +++ b/src/language/LanguageManager.js @@ -443,6 +443,7 @@ 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. * @param {!string} extension An extensionless file name used by this language + * @return {boolean} Whether adding the file name was successful or not */ Language.prototype.addFileName = function (name) { name = name.toLowerCase();