Address review comments

dangoor · dangoor · commit 3669cfd813d7 · 2014-04-23T12:01:38.000-04:00
diff --git a/src/language/LanguageManager.js b/src/language/LanguageManager.js
@@ -134,28 +134,29 @@ define(function (require, exports, module) {
         _modeToLanguageMap              = {},
         _ready;
     
+    // Constants
+    
+    var _EXTENSION_MAP_PREF = "language.fileExtensions",
+        _NAME_MAP_PREF      = "language.fileNames";
+    
     // Tracking for changes to mappings made by preferences
-    var _prefState = {
-        "language.fileExtensions": {
-            last: {},
-            overridden: {},
-            add: "addFileExtension",
-            remove: "removeFileExtension",
-            get: "getLanguageForExtension"
-        },
-        "language.fileNames": {
-            last: {},
-            overridden: {},
-            add: "addFileName",
-            remove: "removeFileName",
-            get: "getLanguageForPath"
-        }
-    };
+    var _prefState = {};
     
-    // Constants
+    _prefState[_EXTENSION_MAP_PREF] = {
+        last: {},
+        overridden: {},
+        add: "addFileExtension",
+        remove: "removeFileExtension",
+        get: "getLanguageForExtension"
+    };
     
-    var EXTENSION_MAP_PREF = "language.fileExtensions",
-        NAME_MAP_PREF      = "language.fileNames";
+    _prefState[_NAME_MAP_PREF] = {
+        last: {},
+        overridden: {},
+        add: "addFileName",
+        remove: "removeFileName",
+        get: "getLanguageForPath"
+    };
     
     // Helper functions
     
@@ -888,6 +889,19 @@ define(function (require, exports, module) {
      * @private
      * 
      * Updates extension and filename mappings from languages based on the current preferences values.
+     * 
+     * The preferences look like this in a prefs file:
+     * 
+     * Map *.foo to javascript, *.vm to html
+     * "language.fileExtensions": {
+     *     "foo": "javascript",
+     *     "vm": "html"
+     * }
+     * 
+     * Map "Gemfile" to ruby:
+     * "language.fileNames": {
+     *     "Gemfile": "ruby"
+     * }
      */
     function _updateFromPrefs(pref) {
         var newMapping = PreferencesManager.get(pref) || {},
@@ -981,23 +995,27 @@ define(function (require, exports, module) {
         // The fallback language for unknown modes and file extensions
         _fallbackLanguage = getLanguage("unknown");
         
-        // There is a circular dependency between FileUtils and LanuageManager which
+        // There is a circular dependency between FileUtils and LanguageManager which
         // was introduced in 254b01e2f2eebea4416026d0f40d017b8ca6dbc9
         // and may be preventing us from importing PreferencesManager (which also
         // depends on FileUtils) here. Using the async form of require fixes this.
         require(["preferences/PreferencesManager"], function (pm) {
             PreferencesManager = pm;
-            _updateFromPrefs(EXTENSION_MAP_PREF);
-            _updateFromPrefs(NAME_MAP_PREF);
-            pm.definePreference(EXTENSION_MAP_PREF, "object").on("change", function () {
-                _updateFromPrefs(EXTENSION_MAP_PREF);
+            _updateFromPrefs(_EXTENSION_MAP_PREF);
+            _updateFromPrefs(_NAME_MAP_PREF);
+            pm.definePreference(_EXTENSION_MAP_PREF, "object").on("change", function () {
+                _updateFromPrefs(_EXTENSION_MAP_PREF);
             });
-            pm.definePreference(NAME_MAP_PREF, "object").on("change", function () {
-                _updateFromPrefs(NAME_MAP_PREF);
+            pm.definePreference(_NAME_MAP_PREF, "object").on("change", function () {
+                _updateFromPrefs(_NAME_MAP_PREF);
             });
         });
     });
     
+    // Private for unit tests
+    exports._EXTENSION_MAP_PREF = _EXTENSION_MAP_PREF;
+    exports._NAME_MAP_PREF      = _NAME_MAP_PREF;
+    
     // Public methods
     exports.ready                   = _ready;
     exports.defineLanguage          = defineLanguage;
diff --git a/test/spec/LanguageManager-test.js b/test/spec/LanguageManager-test.js
@@ -676,62 +676,57 @@ define(function (require, exports, module) {
             it("should be able to add extension mappings via a preference", function () {
                 var language = LanguageManager.getLanguageForExtension("foo");
                 expect(language).toBeUndefined();
-                PreferencesManager.set("language.fileExtensions", {
+                PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, {
                     foo: "javascript"
                 });
                 language = LanguageManager.getLanguageForExtension("foo");
                 expect(language.getId()).toBe("javascript");
-            });
-            
-            it("should remove extension mappings when preferences change", function () {
-                var language = LanguageManager.getLanguageForExtension("foo");
-                expect(language).toBeDefined();
-                PreferencesManager.set("language.fileExtensions", { });
+                PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, { });
                 language = LanguageManager.getLanguageForExtension("foo");
                 expect(language).toBeUndefined();
             });
             
             it("should manage overridden default extensions", function () {
-                PreferencesManager.set("language.fileExtensions", {
+                PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, {
                     js: "html"
                 });
                 var language = LanguageManager.getLanguageForExtension("js");
                 expect(language.getId()).toBe("html");
-                PreferencesManager.set("language.fileExtensions", {
+                PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, {
                     js: "php"
                 });
                 language = LanguageManager.getLanguageForExtension("js");
                 expect(language.getId()).toBe("php");
-                PreferencesManager.set("language.fileExtensions", { });
+                PreferencesManager.set(LanguageManager._EXTENSION_MAP_PREF, { });
                 language = LanguageManager.getLanguageForExtension("js");
                 expect(language.getId()).toBe("javascript");
             });
             
             it("should be able to manage file name mappings via a preference", function () {
                 var language = LanguageManager.getLanguageForPath("/bar/Foofile");
                 expect(language.getId()).toBe("unknown");
-                PreferencesManager.set("language.fileNames", {
+                PreferencesManager.set(LanguageManager._NAME_MAP_PREF, {
                     "Foofile": "javascript"
                 });
                 language = LanguageManager.getLanguageForPath("/bar/Foofile");
                 expect(language.getId()).toBe("javascript");
-                PreferencesManager.set("language.fileNames", { });
+                PreferencesManager.set(LanguageManager._NAME_MAP_PREF, { });
                 language = LanguageManager.getLanguageForPath("/bar/Foofile");
                 expect(language.getId()).toBe("unknown");
             });
             
             it("should manage overridden default file names", function () {
-                PreferencesManager.set("language.fileNames", {
+                PreferencesManager.set(LanguageManager._NAME_MAP_PREF, {
                     Gemfile: "python"
                 });
                 var language = LanguageManager.getLanguageForPath("Gemfile");
                 expect(language.getId()).toBe("python");
-                PreferencesManager.set("language.fileNames", {
+                PreferencesManager.set(LanguageManager._NAME_MAP_PREF, {
                     Gemfile: "php"
                 });
                 language = LanguageManager.getLanguageForPath("Gemfile");
                 expect(language.getId()).toBe("php");
-                PreferencesManager.set("language.fileNames", { });
+                PreferencesManager.set(LanguageManager._NAME_MAP_PREF, { });
                 language = LanguageManager.getLanguageForPath("Gemfile");
                 expect(language.getId()).toBe("ruby");
             });
