Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: user settings to control diagnostics reporting cross view files #734

Merged
merged 4 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .changeset/ten-rivers-pull.md
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe("aggregation binding", () => {
server: "off",
},
SplitAttributesOnFormat: true,
LimitUniqueIdDiagnostics: false,
};

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,
LimitUniqueIdDiagnostics: false,
};

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,
LimitUniqueIdDiagnostics: false,
};

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,
LimitUniqueIdDiagnostics: false,
};

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,
LimitUniqueIdDiagnostics: false,
};

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 limitUniqueIdsDiagReport = settings.LimitUniqueIdDiagnostics;
if (limitUniqueIdsDiagReport) {
validateIdsOfOpenDocument(document, context);
} else {
await validateIdsOfOpenDocuments();
}
}
});

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 &
LimitUniqueIdDiagnostics &
FormatterSettings;

export interface WebServerSettings {
SAPUI5WebServer?: string;
}
export interface LimitUniqueIdDiagnostics {
LimitUniqueIdDiagnostics: 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,
LimitUniqueIdDiagnostics: false,
};
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,
LimitUniqueIdDiagnostics: false,
};
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,
LimitUniqueIdDiagnostics: false,
};
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,
LimitUniqueIdDiagnostics: false,
};
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,
LimitUniqueIdDiagnostics: false,
})
);
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,
LimitUniqueIdDiagnostics: false,
})
);
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,
LimitUniqueIdDiagnostics: false,
};
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,
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));
Expand All @@ -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));
Expand All @@ -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));

Expand All @@ -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));
Expand All @@ -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(
Expand All @@ -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);
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

- `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
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.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",
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
Loading