Fix overrides priority (#1027)

* Respect user-set `priority` over `priority` set by `overrides.json`

* Add a test for merging user settings with defaults

Author: krassowski

packages/jupyterlab-lsp/src/settings.spec.ts

@@ -0,0 +1,55 @@
+import { SettingsSchemaManager } from './settings';
+
+const DEAULT_SERVER_PRIORITY = 50;
+
+describe('SettingsSchemaManager', () => {
+  describe('#mergeByServer()', () => {
+    it('prioritises user `priority` over the default', () => {
+      const defaults = {
+        pyright: {
+          priority: 500,
+          serverSettings: {}
+        }
+      };
+      const user = {
+        pyright: {
+          priority: 100,
+          serverSettings: {}
+        }
+      };
+      const result = SettingsSchemaManager.mergeByServer(defaults, user);
+      expect(result.pyright.priority).toBe(100);
+    });
+    it('should use default `priority` if no user value', () => {
+      const defaults = {
+        pyright: {
+          priority: 500,
+          serverSettings: {}
+        }
+      };
+      const user = {
+        pyright: {
+          serverSettings: {}
+        }
+      };
+      const result = SettingsSchemaManager.mergeByServer(defaults, user);
+      expect(result.pyright.priority).toBe(500);
+    });
+    it('should prefer `priority` from `overrides.json` (which is recorded in defaults) if user value is set to the default value', () => {
+      const defaults = {
+        pyright: {
+          priority: 500,
+          serverSettings: {}
+        }
+      };
+      const user = {
+        pyright: {
+          priority: DEAULT_SERVER_PRIORITY,
+          serverSettings: {}
+        }
+      };
+      const result = SettingsSchemaManager.mergeByServer(defaults, user);
+      expect(result.pyright.priority).toBe(500);
+    });
+  });
+});

packages/jupyterlab-lsp/src/settings.ts

@@ -48,6 +48,11 @@ function isJSONProperty(obj: unknown): obj is IJSONProperty {
  */
 type LanguageServerSettings = Record<string, ServerSchemaWrapper>;
 
+/**
+ * Default server priority; this should match the value defined in `plugin.json` schema.
+ */
+const DEAULT_SERVER_PRIORITY = 50;
+
 /**
  * Get default values from JSON Schema properties field.
  */
@@ -396,7 +401,7 @@ export class SettingsSchemaManager {
         composite.language_servers
       );
 
-      composite.language_servers = this._mergeByServer(
+      composite.language_servers = SettingsSchemaManager.mergeByServer(
         collapsedDefaults.settings,
         collapsedUser.settings
       );
@@ -552,7 +557,7 @@ export class SettingsSchemaManager {
     };
   }
 
-  private _mergeByServer(
+  static mergeByServer(
     defaults: LanguageServerSettings,
     userSettings: LanguageServerSettings
   ): LanguageServerSettings {
@@ -565,9 +570,19 @@ export class SettingsSchemaManager {
         // nothing to merge with
         result[serverKey] = JSONExt.deepCopy(serverSettingsGroup);
       } else {
+        // priority should come from (a) user (b) overrides (c) fallback default;
+        // unfortunately the user and default values get merged in the form so we
+        // cannot distinguish (a) from (c); as a workaround we can compare its value
+        // with the default value.
+        const userOrDefaultPriority = serverSettingsGroup.priority;
+        const isPriorityUserSet =
+          typeof userOrDefaultPriority !== 'undefined' &&
+          userOrDefaultPriority !== DEAULT_SERVER_PRIORITY;
+        const priority = isPriorityUserSet
+          ? userOrDefaultPriority
+          : result[serverKey].priority ?? DEAULT_SERVER_PRIORITY;
         const merged: Required<ServerSchemaWrapper> = {
-          priority: (result[serverKey].priority ||
-            serverSettingsGroup.priority) as any,
+          priority,
           // `serverSettings` entries are expected to be flattened to dot notation here.
           serverSettings: {
             ...(result[serverKey].serverSettings || {}),