From 3abf9a622098ba1100615290c1617c20cdc1735f Mon Sep 17 00:00:00 2001 From: Maruf Rasully <100434800+marufrasully@users.noreply.github.com> Date: Wed, 16 Oct 2024 09:43:25 +0200 Subject: [PATCH] feat: user settings to control diagnostics reporting cross view files (#734) * feat: user settings to control diagnostics reporting cross different view files * fix: add change set * fix: review comment * fix: update readme file --- .changeset/ten-rivers-pull.md | 12 ++++ .../completion/aggregation-binding.test.ts | 1 + .../completion/property-binding-info.test.ts | 1 + packages/context/api.d.ts | 1 + .../completion/providers/context-path.test.ts | 1 + .../completion/providers/filter-bar.test.ts | 1 + .../completion/providers/meta-path.test.ts | 1 + packages/language-server/src/server.ts | 37 ++++++++-- packages/settings/api.d.ts | 4 ++ packages/settings/src/settings.ts | 1 + packages/settings/test/unit/settings.test.ts | 15 ++++ .../vscode-ui5-language-assistant/README.md | 4 ++ .../package.json | 6 ++ .../test/manual-tests/unique-control-ids.md | 28 +++++++- packages/xml-views-validation/package.json | 1 + .../src/validators/non-unique-id.ts | 56 +++++++++++++-- .../unit/validators/non-unique-id.test.ts | 68 +++++++++++++++++++ 17 files changed, 225 insertions(+), 13 deletions(-) create mode 100644 .changeset/ten-rivers-pull.md diff --git a/.changeset/ten-rivers-pull.md b/.changeset/ten-rivers-pull.md new file mode 100644 index 000000000..dbd00843d --- /dev/null +++ b/.changeset/ten-rivers-pull.md @@ -0,0 +1,12 @@ +--- +"@ui5-language-assistant/vscode-ui5-language-assistant-bas-ext": patch +"vscode-ui5-language-assistant": patch +"@ui5-language-assistant/xml-views-completion": patch +"@ui5-language-assistant/language-server": patch +"@ui5-language-assistant/settings": patch +"@ui5-language-assistant/binding": patch +"@ui5-language-assistant/context": patch +"@ui5-language-assistant/fe": patch +--- + +feat: User settings to control diagnostics reporting cross view files diff --git a/packages/binding/test/unit/services/completion/aggregation-binding.test.ts b/packages/binding/test/unit/services/completion/aggregation-binding.test.ts index 8cb5b7966..28937a0e9 100644 --- a/packages/binding/test/unit/services/completion/aggregation-binding.test.ts +++ b/packages/binding/test/unit/services/completion/aggregation-binding.test.ts @@ -39,6 +39,7 @@ describe("aggregation binding", () => { server: "off", }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; beforeAll(async function () { diff --git a/packages/binding/test/unit/services/completion/property-binding-info.test.ts b/packages/binding/test/unit/services/completion/property-binding-info.test.ts index 436be27fe..1915ffc37 100644 --- a/packages/binding/test/unit/services/completion/property-binding-info.test.ts +++ b/packages/binding/test/unit/services/completion/property-binding-info.test.ts @@ -44,6 +44,7 @@ describe("index", () => { server: "off", }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; beforeAll(async function () { diff --git a/packages/context/api.d.ts b/packages/context/api.d.ts index 0be861909..92e3c9f03 100644 --- a/packages/context/api.d.ts +++ b/packages/context/api.d.ts @@ -5,5 +5,6 @@ export type { ManifestDetails, YamlDetails, ProjectKind, + ControlIdLocation, } from "./src/types"; export type { Manifest } from "@sap-ux/project-access"; diff --git a/packages/fe/test/unit/services/completion/providers/context-path.test.ts b/packages/fe/test/unit/services/completion/providers/context-path.test.ts index 690941c6c..86f69bee0 100644 --- a/packages/fe/test/unit/services/completion/providers/context-path.test.ts +++ b/packages/fe/test/unit/services/completion/providers/context-path.test.ts @@ -45,6 +45,7 @@ describe("contextPath attribute value completion", () => { server: "off", }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; const annotationSnippetCDS = ` diff --git a/packages/fe/test/unit/services/completion/providers/filter-bar.test.ts b/packages/fe/test/unit/services/completion/providers/filter-bar.test.ts index 6db56b194..9dc2b9196 100644 --- a/packages/fe/test/unit/services/completion/providers/filter-bar.test.ts +++ b/packages/fe/test/unit/services/completion/providers/filter-bar.test.ts @@ -43,6 +43,7 @@ describe("filterBar id attribute value completion", () => { server: "off", }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; const annotationSnippetCDS = ` diff --git a/packages/fe/test/unit/services/completion/providers/meta-path.test.ts b/packages/fe/test/unit/services/completion/providers/meta-path.test.ts index 848b41519..4c081c160 100644 --- a/packages/fe/test/unit/services/completion/providers/meta-path.test.ts +++ b/packages/fe/test/unit/services/completion/providers/meta-path.test.ts @@ -48,6 +48,7 @@ describe("metaPath attribute value completion", () => { server: "off", }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; const annotationSnippetCDS = ` diff --git a/packages/language-server/src/server.ts b/packages/language-server/src/server.ts index 59f278749..1e802c3e4 100644 --- a/packages/language-server/src/server.ts +++ b/packages/language-server/src/server.ts @@ -24,6 +24,7 @@ import { getSettingsForDocument, setConfigurationSettings, Settings, + getConfigurationSettings, } from "@ui5-language-assistant/settings"; import { commands } from "@ui5-language-assistant/user-facing-text"; import { ServerInitializationOptions } from "../api"; @@ -44,6 +45,7 @@ import { reactOnViewFileChange, reactOnPackageJson, isContext, + Context, } from "@ui5-language-assistant/context"; import { diagnosticToCodeActionFix } from "./quick-fix"; import { executeCommand } from "./commands"; @@ -296,6 +298,28 @@ const validateIdsOfOpenDocuments = async (): Promise => { connection.sendDiagnostics({ uri: document.uri, diagnostics }); } }; +/** + * Validates the IDs of the open document and sends the diagnostics to the client + * @param {TextDocument} document - The open document to be validated + * @param {Context} context - The context containing additional information + * @returns {void} + */ +const validateIdsOfOpenDocument = ( + document: TextDocument, + context: Context +): void => { + const idDiagnostics = getXMLViewIdDiagnostics({ + document, + context, + }); + let diagnostics = documentsDiagnostics.get(document.uri) ?? []; + diagnostics = diagnostics.concat(idDiagnostics); + + getLogger().trace("computed diagnostics", { + diagnostics, + }); + connection.sendDiagnostics({ uri: document.uri, diagnostics }); +}; async function validateOpenDocumentsOnDidChangeWatchedFiles( changes: FileEvent[] @@ -391,7 +415,13 @@ documents.onDidChangeContent(async (changeEvent): Promise => { context, }); documentsDiagnostics.set(document.uri, diagnostics); - await validateIdsOfOpenDocuments(); + const settings = getConfigurationSettings(); + const limitUniqueIdsDiagReport = settings.LimitUniqueIdDiagnostics; + if (limitUniqueIdsDiagReport) { + validateIdsOfOpenDocument(document, context); + } else { + await validateIdsOfOpenDocuments(); + } } }); @@ -466,7 +496,7 @@ function ensureDocumentSettingsUpdated(resource: string): void { } } -connection.onDidChangeConfiguration((change) => { +connection.onDidChangeConfiguration(async (change) => { getLogger().debug("`onDidChangeConfiguration` event"); if (hasConfigurationCapability) { getLogger().trace("Reset all cached document settings"); @@ -487,9 +517,8 @@ connection.onDidChangeConfiguration((change) => { }); setConfigurationSettings(ui5LangAssistSettings); } - // In the future we might want to // re-validate the files related to the `cached document settings`. - + await validateIdsOfOpenDocuments(); // `setLogLevel` will ignore `undefined` values setLogLevel(change?.settings?.UI5LanguageAssistant?.logging?.level); }); diff --git a/packages/settings/api.d.ts b/packages/settings/api.d.ts index d5847a4a9..9509ae3f1 100644 --- a/packages/settings/api.d.ts +++ b/packages/settings/api.d.ts @@ -13,11 +13,15 @@ export type Settings = CodeAssistSettings & TraceSettings & LoggingSettings & WebServerSettings & + LimitUniqueIdDiagnostics & FormatterSettings; export interface WebServerSettings { SAPUI5WebServer?: string; } +export interface LimitUniqueIdDiagnostics { + LimitUniqueIdDiagnostics: boolean; +} export interface FormatterSettings { SplitAttributesOnFormat: boolean; } diff --git a/packages/settings/src/settings.ts b/packages/settings/src/settings.ts index b81063215..a8cd3f26e 100644 --- a/packages/settings/src/settings.ts +++ b/packages/settings/src/settings.ts @@ -9,6 +9,7 @@ const defaultSettings: Settings = { logging: { level: "error" }, SAPUI5WebServer: "", SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; deepFreezeStrict(defaultSettings); diff --git a/packages/settings/test/unit/settings.test.ts b/packages/settings/test/unit/settings.test.ts index 88a068d56..9657fa537 100644 --- a/packages/settings/test/unit/settings.test.ts +++ b/packages/settings/test/unit/settings.test.ts @@ -46,6 +46,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setGlobalSettings(globalSettings); const docSettings = await getSettingsForDocument("doc1"); @@ -58,6 +59,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setGlobalSettings(globalSettings); const docSettings = await getSettingsForDocument("doc1"); @@ -75,6 +77,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setSettingsForDocument("doc1", Promise.resolve(docSettings)); const result = await getSettingsForDocument("doc1"); @@ -91,6 +94,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }) ); expect(hasSettingsForDocument("doc1")).toBeTrue(); @@ -107,6 +111,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }) ); expect(hasSettingsForDocument("doc1")).toBeFalse(); @@ -120,6 +125,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setSettingsForDocument("doc1", Promise.resolve(docSettings)); expect(await getSettingsForDocument("doc1")).toStrictEqual(docSettings); @@ -131,12 +137,14 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; const docSettings2 = { codeAssist: { deprecated: true, experimental: false }, trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setSettingsForDocument("doc1", Promise.resolve(docSettings1)); setSettingsForDocument("doc1", Promise.resolve(docSettings2)); @@ -156,12 +164,14 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; const docSettings2 = { codeAssist: { deprecated: true, experimental: false }, trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setSettingsForDocument("doc1", Promise.resolve(docSettings1)); setSettingsForDocument("doc2", Promise.resolve(docSettings2)); @@ -187,6 +197,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setSettingsForDocument("doc1", Promise.resolve(docSettings)); @@ -202,12 +213,14 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; const docSettings2 = { codeAssist: { deprecated: true, experimental: false }, trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setSettingsForDocument("doc1", Promise.resolve(docSettings1)); setSettingsForDocument("doc2", Promise.resolve(docSettings2)); @@ -224,6 +237,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setGlobalSettings(globalSettings); expect(await getSettingsForDocument("doc1")).toStrictEqual( @@ -239,6 +253,7 @@ describe("settings utilities", () => { trace: { server: "off" as const }, logging: { level: "off" as const }, SplitAttributesOnFormat: true, + LimitUniqueIdDiagnostics: false, }; setConfigurationSettings(configSettings); expect(getConfigurationSettings()).toStrictEqual(configSettings); diff --git a/packages/vscode-ui5-language-assistant/README.md b/packages/vscode-ui5-language-assistant/README.md index 5bf31d5dd..d779bcf56 100644 --- a/packages/vscode-ui5-language-assistant/README.md +++ b/packages/vscode-ui5-language-assistant/README.md @@ -95,6 +95,10 @@ and cannot be configured by the end user. - References to filter bars that cannot be found in current file - Use of context paths that do not lead to the annotations of types expected for the given building block +#### Relevant User/Workspace + +- `UI5LanguageAssistant.LimitUniqueIdDiagnostics` is set off by default to check all `*.fragment.xml` and `*.view.xml` files for unique IDs. If you like to limit this validation to the current file, set it on. + ### XML View Quick Fix #### Description diff --git a/packages/vscode-ui5-language-assistant/package.json b/packages/vscode-ui5-language-assistant/package.json index 125af8055..7b27ee3ce 100644 --- a/packages/vscode-ui5-language-assistant/package.json +++ b/packages/vscode-ui5-language-assistant/package.json @@ -90,6 +90,12 @@ "default": true, "markdownDescription": "Put each attribute on a new line when formatting `*.view.xml` or `*.fragment.xml`." }, + "UI5LanguageAssistant.LimitUniqueIdDiagnostics": { + "type": "boolean", + "scope": "window", + "default": false, + "markdownDescription": "Limit diagnostics for unique IDs to current `*.view.xml` or `*.fragment.xml` file" + }, "UI5LanguageAssistant.SAPUI5WebServer": { "type": "string", "scope": "window", diff --git a/packages/vscode-ui5-language-assistant/test/manual-tests/unique-control-ids.md b/packages/vscode-ui5-language-assistant/test/manual-tests/unique-control-ids.md index edf72e258..c8bcca627 100644 --- a/packages/vscode-ui5-language-assistant/test/manual-tests/unique-control-ids.md +++ b/packages/vscode-ui5-language-assistant/test/manual-tests/unique-control-ids.md @@ -2,7 +2,7 @@ ## Associated user stories: -[#646](https://github.com/SAP/ui5-language-assistant/issues/646) Generate ID's: make this unique for a whole project +[#646](https://github.com/SAP/ui5-language-assistant/issues/646) Generate ID's: make this unique for a whole project [#729](https://github.com/SAP/ui5-language-assistant/issues/729) User settings to control diagnostics reporting cross different view files ## Install latest UI5 Language Assistant @@ -61,7 +61,7 @@ You can navigate to related file where identical ids are used **Note:** For this step test, we use `test-packages/framework/projects/adp.test` project 1. Open `actionToolbar.fragment.xml` file which is located under `adp.test/webapp/changes/fragments` folder -2. Copy and replace snippet below with `` +2. Copy and replace snippet below with `` ```xml @@ -77,3 +77,27 @@ You can navigate to related file where identical ids are used ##### Note You can navigate to related file where identical ids are used + +## User setting to control diagnostics reporting + +1. Open user settings by clicking `view>Command Pallette...` and type `Preferences: Open User settings`. Select first option. Search for `Report Non Unique Ids Cross View Files` and uncheck checkbox. (By default it reports diagnostics cross view files) +2. Open `Main.view.xml` file which is located under `app/manage_travels/webapp/ext/main` folder +3. Copy and paste snippet below inside `` tag + +```xml + + + + `; + // modify context + const xmlSnippet02 = ` + + + + `; + + const CustomSectionSegments = [ + "app", + "manage_travels", + "webapp", + "ext", + "fragment", + "CustomSection.fragment.xml", + ]; + await testFramework.updateFile(CustomSectionSegments, xmlSnippet02); + const { context, offsetRanges, documentPath } = await getParam( + xmlSnippet + ); + const offset = offsetRanges[documentPath]; + // act + const result = validateNonUniqueID(context); + // assert + expect(result).toEqual([ + { + issueType: "base", + kind: "NonUniqueIDIssue", + message: buildMessage(NON_UNIQUE_ID.msg, "DUPLICATE"), + severity: "error", + offsetRange: offset[0], + identicalIDsRanges: [], // empty identical id ranges => no cross view files + }, + { + issueType: "base", + kind: "NonUniqueIDIssue", + message: buildMessage(NON_UNIQUE_ID.msg, "DUPLICATE"), + severity: "error", + offsetRange: offset[1], + identicalIDsRanges: [], // empty identical id ranges => no cross view files + }, + ]); + }); }); describe("negative edge cases", () => { it("will not detect issues for duplicate attribute keys that are not `id`", async () => {