Skip to content

Commit

Permalink
feat: user settings to control diagnostics reporting cross different …
Browse files Browse the repository at this point in the history
…view files
  • Loading branch information
marufrasully committed Oct 14, 2024
1 parent 3fa9e9b commit 20abfe1
Show file tree
Hide file tree
Showing 16 changed files with 213 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe("aggregation binding", () => {
server: "off",
},
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};

beforeAll(async function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe("index", () => {
server: "off",
},
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};

beforeAll(async function () {
Expand Down
1 change: 1 addition & 0 deletions packages/context/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ export type {
ManifestDetails,
YamlDetails,
ProjectKind,
ControlIdLocation,
} from "./src/types";
export type { Manifest } from "@sap-ux/project-access";
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ describe("contextPath attribute value completion", () => {
server: "off",
},
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};

const annotationSnippetCDS = `
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ describe("filterBar id attribute value completion", () => {
server: "off",
},
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};

const annotationSnippetCDS = `
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ describe("metaPath attribute value completion", () => {
server: "off",
},
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};

const annotationSnippetCDS = `
Expand Down
37 changes: 33 additions & 4 deletions packages/language-server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -44,6 +45,7 @@ import {
reactOnViewFileChange,
reactOnPackageJson,
isContext,
Context,
} from "@ui5-language-assistant/context";
import { diagnosticToCodeActionFix } from "./quick-fix";
import { executeCommand } from "./commands";
Expand Down Expand Up @@ -296,6 +298,28 @@ const validateIdsOfOpenDocuments = async (): Promise<void> => {
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[]
Expand Down Expand Up @@ -391,7 +415,13 @@ documents.onDidChangeContent(async (changeEvent): Promise<void> => {
context,
});
documentsDiagnostics.set(document.uri, diagnostics);
await validateIdsOfOpenDocuments();
const settings = getConfigurationSettings();
const reportNonUniqueIds = settings.ReportNonUniqueIdsCrossViewFiles;
if (reportNonUniqueIds) {
await validateIdsOfOpenDocuments();
} else {
validateIdsOfOpenDocument(document, context);
}
}
});

Expand Down Expand Up @@ -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");
Expand All @@ -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);
});
Expand Down
4 changes: 4 additions & 0 deletions packages/settings/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@ export type Settings = CodeAssistSettings &
TraceSettings &
LoggingSettings &
WebServerSettings &
NonUniqueIdsSettings &
FormatterSettings;

export interface WebServerSettings {
SAPUI5WebServer?: string;
}
export interface NonUniqueIdsSettings {
ReportNonUniqueIdsCrossViewFiles: boolean;
}
export interface FormatterSettings {
SplitAttributesOnFormat: boolean;
}
Expand Down
1 change: 1 addition & 0 deletions packages/settings/src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const defaultSettings: Settings = {
logging: { level: "error" },
SAPUI5WebServer: "",
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
deepFreezeStrict(defaultSettings);

Expand Down
15 changes: 15 additions & 0 deletions packages/settings/test/unit/settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setGlobalSettings(globalSettings);
const docSettings = await getSettingsForDocument("doc1");
Expand All @@ -58,6 +59,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setGlobalSettings(globalSettings);
const docSettings = await getSettingsForDocument("doc1");
Expand All @@ -75,6 +77,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setSettingsForDocument("doc1", Promise.resolve(docSettings));
const result = await getSettingsForDocument("doc1");
Expand All @@ -91,6 +94,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
})
);
expect(hasSettingsForDocument("doc1")).toBeTrue();
Expand All @@ -107,6 +111,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
})
);
expect(hasSettingsForDocument("doc1")).toBeFalse();
Expand All @@ -120,6 +125,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setSettingsForDocument("doc1", Promise.resolve(docSettings));
expect(await getSettingsForDocument("doc1")).toStrictEqual(docSettings);
Expand All @@ -131,12 +137,14 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
const docSettings2 = {
codeAssist: { deprecated: true, experimental: false },
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setSettingsForDocument("doc1", Promise.resolve(docSettings1));
setSettingsForDocument("doc1", Promise.resolve(docSettings2));
Expand All @@ -156,12 +164,14 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
const docSettings2 = {
codeAssist: { deprecated: true, experimental: false },
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setSettingsForDocument("doc1", Promise.resolve(docSettings1));
setSettingsForDocument("doc2", Promise.resolve(docSettings2));
Expand All @@ -187,6 +197,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setSettingsForDocument("doc1", Promise.resolve(docSettings));

Expand All @@ -202,12 +213,14 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
const docSettings2 = {
codeAssist: { deprecated: true, experimental: false },
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setSettingsForDocument("doc1", Promise.resolve(docSettings1));
setSettingsForDocument("doc2", Promise.resolve(docSettings2));
Expand All @@ -224,6 +237,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setGlobalSettings(globalSettings);
expect(await getSettingsForDocument("doc1")).toStrictEqual(
Expand All @@ -239,6 +253,7 @@ describe("settings utilities", () => {
trace: { server: "off" as const },
logging: { level: "off" as const },
SplitAttributesOnFormat: true,
ReportNonUniqueIdsCrossViewFiles: true,
};
setConfigurationSettings(configSettings);
expect(getConfigurationSettings()).toStrictEqual(configSettings);
Expand Down
4 changes: 4 additions & 0 deletions packages/vscode-ui5-language-assistant/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 settings for Duplicate ID tags

- `UI5LanguageAssistant.ReportNonUniqueIdsCrossViewFiles` is set on by default and report diagnostics cross view files. Settings it to false, reports only on a single file

### XML View Quick Fix

#### Description
Expand Down
6 changes: 6 additions & 0 deletions packages/vscode-ui5-language-assistant/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@
"default": true,
"markdownDescription": "Put each attribute on a new line when formatting `*.view.xml` or `*.fragment.xml`."
},
"UI5LanguageAssistant.ReportNonUniqueIdsCrossViewFiles": {
"type": "boolean",
"scope": "window",
"default": true,
"markdownDescription": "Diagnostic report for non unique ID on `*.view.xml` or `*.fragment.xml`."
},
"UI5LanguageAssistant.SAPUI5WebServer": {
"type": "string",
"scope": "window",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 `<!-- add your xml here -->`
2. Copy and replace snippet below with `<!-- add your xml here -->`

```xml
<Text id="_IDGenText" >
Expand All @@ -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 `<Content>` tag

```xml
<Text id="_IDGenText" >
<Button id="_IDGenButton" />
```

4. Check that no diagnostic is reported as there is not duplicate id
5. Create a new file e.g `MyTest.view.xml` under webapp and copy paste content of `Main.view.xml` file in `MyTest.view.xml`
6. Check that no diagnostics is reported for duplicate ids cross view files
7. Add `<Text id="_IDGenText" >` to `Main.view.xml`
8. Check that diagnostic is reported for duplicate ids of same file
9. Reset checkbox of `Report Non Unique Ids Cross View Files`. See instruction from #1 above
10. Check that diagnostics are reported for duplicate ids across view files

## Quick fix with "Report Non Unique Ids Cross View Files" user settings

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. [Follow steps](#quick-fix). Despite user settings, behavior should not be affected
1 change: 1 addition & 0 deletions packages/xml-views-validation/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"license": "Apache-2.0",
"typings": "./api.d.ts",
"dependencies": {
"@ui5-language-assistant/settings": "4.0.9",
"@ui5-language-assistant/constant": "0.0.1",
"@ui5-language-assistant/context": "4.0.30",
"@ui5-language-assistant/logic-utils": "4.0.20",
Expand Down
Loading

0 comments on commit 20abfe1

Please sign in to comment.