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: add XML validation and quickfix for hardcoded UI text #491

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@
"source-map-support": "0.5.19",
"typescript": "3.9.7",
"webpack": "5.36.2",
"webpack-cli": "4.4.0"
"webpack-cli": "4.4.0",
"properties-file": "2.1.7"
},
"lint-staged": {
"*.{js,ts,md,json}": [
Expand Down
16 changes: 16 additions & 0 deletions packages/language-server/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { commands } from "@ui5-language-assistant/user-facing-text";
import {
executeQuickFixStableIdCommand,
executeQuickFixFileStableIdCommand,
executeQuickFixHardcodedI18nStringCommand,
} from "./quick-fix";
import { track } from "./swa";

Expand Down Expand Up @@ -44,6 +45,21 @@ export function executeCommand(
track("MANIFEST_STABLE_ID", "multiple");
return;
}
case commands.QUICK_FIX_HARDCODED_I18N_STRING_ERROR.name: {
const change = executeQuickFixHardcodedI18nStringCommand({
// Assumption that this command has the following arguments.
// We passed them when the command was created.
documentUri: params.arguments[0],
documentVersion: params.arguments[1],
quickFixReplaceRange: params.arguments[2],
quickFixNewText: params.arguments[3],
});
connection.workspace.applyEdit({
documentChanges: change,
});
track("MANIFEST_HARDCODED_I18N_STRING", "single");
return;
}
default:
return undefined;
}
Expand Down
85 changes: 84 additions & 1 deletion packages/language-server/src/quick-fix.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ import {
} from "@ui5-language-assistant/xml-views-validation";
import { UI5SemanticModel } from "@ui5-language-assistant/semantic-model-types";
import { computeQuickFixStableIdInfo } from "@ui5-language-assistant/xml-views-quick-fix";
import { computeQuickFixHardcodedI18nStringInfo } from "@ui5-language-assistant/xml-views-quick-fix";
import {
validations,
commands,
} from "@ui5-language-assistant/user-facing-text";
import { LSPRangeToOffsetRange, offsetRangeToLSPRange } from "./range-utils";
import { Property } from "properties-file";

type QuickFixStableIdLSPInfo = {
newText: string;
Expand All @@ -32,7 +34,8 @@ type QuickFixStableIdLSPInfo = {
export function diagnosticToCodeActionFix(
document: TextDocument,
diagnostics: Diagnostic[],
ui5Model: UI5SemanticModel
ui5Model: UI5SemanticModel,
resourceBundle: Property[]
): CodeAction[] {
const documentText = document.getText();
// We prefer to parse the document again to avoid cache state handling
Expand All @@ -49,6 +52,16 @@ export function diagnosticToCodeActionFix(
ui5Model,
});
}
case validations.HARDCODED_I18N_STRING.code: {
// hardcoded i18n string
return computeCodeActionsForQuickFixHardcodedI18nString({
document,
xmlDocument: xmlDocAst,
hardcodedI18nStringDiagnostic: diagnostic,
ui5Model,
resourceBundle,
});
}
default:
return [];
}
Expand Down Expand Up @@ -191,3 +204,73 @@ export function executeQuickFixFileStableIdCommand(opts: {

return documentEdit;
}

function computeCodeActionsForQuickFixHardcodedI18nString(opts: {
document: TextDocument;
xmlDocument: XMLDocument;
hardcodedI18nStringDiagnostic: Diagnostic;
ui5Model: UI5SemanticModel;
resourceBundle: Property[];
}): CodeAction[] {
const codeActions: CodeAction[] = [];

const errorOffset = LSPRangeToOffsetRange(
opts.hardcodedI18nStringDiagnostic.range,
opts.document
);

const quickFixHardcodedI18nStringInfo = computeQuickFixHardcodedI18nStringInfo(
opts.xmlDocument,
[errorOffset],
opts.resourceBundle
);

const replaceRange = offsetRangeToLSPRange(
quickFixHardcodedI18nStringInfo[0].replaceRange,
opts.document
);

quickFixHardcodedI18nStringInfo[0].newTextSuggestions.forEach(
(suggestion) => {
const codeActionTitle =
commands.QUICK_FIX_HARDCODED_I18N_STRING_ERROR.title +
": " +
suggestion.suggestionValue +
" (" +
suggestion.suggestionKey +
")";
codeActions.push(
CodeAction.create(
codeActionTitle,
Command.create(
commands.QUICK_FIX_HARDCODED_I18N_STRING_ERROR.title,
commands.QUICK_FIX_HARDCODED_I18N_STRING_ERROR.name,
opts.document.uri,
opts.document.version,
replaceRange,
suggestion.newText
),
CodeActionKind.QuickFix
)
);
}
);

return codeActions;
}

export function executeQuickFixHardcodedI18nStringCommand(opts: {
documentUri: string;
documentVersion: number;
quickFixReplaceRange: LSPRange;
quickFixNewText: string;
}): TextDocumentEdit[] {
const documentEdit = [
TextDocumentEdit.create(
{ uri: opts.documentUri, version: opts.documentVersion },
[TextEdit.replace(opts.quickFixReplaceRange, `${opts.quickFixNewText}`)]
),
];

return documentEdit;
}
122 changes: 122 additions & 0 deletions packages/language-server/src/resource-bundle-handling.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { dirname } from "path";
import { maxBy, map, filter } from "lodash";
import { readFile } from "fs-extra";
import { URI } from "vscode-uri";
import globby from "globby";
import { FileChangeType } from "vscode-languageserver";
import { getLogger } from "./logger";
import * as propertiesParser from "properties-file/content";
import { Property } from "properties-file";

type AbsolutePath = string;
type ResourceBundleData = Record<AbsolutePath, Property[]>;
const resourceBundleData: ResourceBundleData = Object.create(null);

export function isResourceBundleDoc(uri: string): boolean {
return uri.endsWith("i18n.properties");
}

export async function initializeResourceBundleData(
workspaceFolderPath: string
): Promise<void[]> {
const resourceBundleDocuments = await findAllResourceBundleDocumentsInWorkspace(
workspaceFolderPath
);

const readResourceBundlePromises = map(
resourceBundleDocuments,
async (resourceBundleDoc) => {
const response = await readResourceBundleFile(resourceBundleDoc);

// Parsing of i18n.properties failed because the file is invalid
if (response !== "INVALID") {
resourceBundleData[resourceBundleDoc] = response;
}
}
);

getLogger().info("resourceBundle data initialized", {
resourceBundleDocuments,
});
return Promise.all(readResourceBundlePromises);
}

export function getResourceBundleData(documentPath: string): Property[] {
const resourceBundleFilesForCurrentFolder = filter(
Object.keys(resourceBundleData),
(resourceBundlePath) =>
documentPath.startsWith(dirname(resourceBundlePath.replace("/i18n", "")))
);

const closestResourceBundlePath = maxBy(
resourceBundleFilesForCurrentFolder,
(resourceBundlePath) => resourceBundlePath.length
);

if (closestResourceBundlePath === undefined) {
return [];
}

return resourceBundleData[closestResourceBundlePath];
}

export async function updateResourceBundleData(
resourceBundleUri: string,
changeType: FileChangeType
): Promise<void> {
getLogger().debug("`updateResourceBundleData` function called", {
resourceBundleUri,
changeType,
});
const resourceBundlePath = URI.parse(resourceBundleUri).fsPath;
switch (changeType) {
case 1: //created
case 2: {
//changed
const response = await readResourceBundleFile(resourceBundleUri);
// Parsing of i18n.properties failed because the file is invalid
// We want to keep last successfully read state - i18n.properties file may be actively edited
if (response !== "INVALID") {
resourceBundleData[resourceBundlePath] = response;
}
return;
}
case 3: //deleted
delete resourceBundleData[resourceBundlePath];
return;
}
}

async function findAllResourceBundleDocumentsInWorkspace(
workspaceFolderPath: string
): Promise<string[]> {
return globby(`${workspaceFolderPath}/**/i18n.properties`).catch((reason) => {
getLogger().error(
`Failed to find all i18n.properties files in current workspace!`,
{
workspaceFolderPath,
reason,
}
);
return [];
});
}

async function readResourceBundleFile(
resourceBundleUri: string
): Promise<Property[] | "INVALID"> {
const resourceBundleContent = await readFile(
URI.parse(resourceBundleUri).fsPath,
"utf-8"
);

let resourceBundleObject: Property[];
try {
resourceBundleObject = propertiesParser.getProperties(resourceBundleContent)
.collection;
} catch (err) {
return "INVALID";
}

return resourceBundleObject;
}
24 changes: 22 additions & 2 deletions packages/language-server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ import {
initializeManifestData,
updateManifestData,
} from "./manifest-handling";
import {
getResourceBundleData,
initializeResourceBundleData,
isResourceBundleDoc,
updateResourceBundleData,
} from "./resource-bundle-handling";
import {
getUI5FrameworkForXMLFile,
isUI5YamlDoc,
Expand All @@ -50,6 +56,7 @@ const connection = createConnection(ProposedFeatures.all);
const documents = new TextDocuments(TextDocument);
let manifestStateInitialized: Promise<void[]> | undefined = undefined;
let ui5yamlStateInitialized: Promise<void[]> | undefined = undefined;
let resourceBundletStateInitialized: Promise<void[]> | undefined = undefined;
let initializationOptions: ServerInitializationOptions | undefined;
let hasConfigurationCapability = false;

Expand All @@ -66,6 +73,9 @@ connection.onInitialize((params: InitializeParams) => {
const workspaceFolderAbsPath = URI.parse(workspaceFolderUri).fsPath;
manifestStateInitialized = initializeManifestData(workspaceFolderAbsPath);
ui5yamlStateInitialized = initializeUI5YamlData(workspaceFolderAbsPath);
resourceBundletStateInitialized = initializeResourceBundleData(
workspaceFolderAbsPath
);
}

// Does the client support the `workspace/configuration` request?
Expand All @@ -92,6 +102,7 @@ connection.onInitialize((params: InitializeParams) => {
commands: [
commands.QUICK_FIX_STABLE_ID_ERROR.name,
commands.QUICK_FIX_STABLE_ID_FILE_ERRORS.name,
commands.QUICK_FIX_HARDCODED_I18N_STRING_ERROR.name,
],
},
},
Expand Down Expand Up @@ -198,6 +209,8 @@ connection.onDidChangeWatchedFiles(async (changeEvent) => {
await updateManifestData(uri, change.type);
} else if (isUI5YamlDoc(uri)) {
await updateUI5YamlData(uri, change.type);
} else if (isResourceBundleDoc(uri)) {
await updateResourceBundleData(uri, change.type);
}
});
});
Expand All @@ -207,12 +220,17 @@ documents.onDidChangeContent(async (changeEvent) => {
if (
manifestStateInitialized === undefined ||
ui5yamlStateInitialized === undefined ||
resourceBundletStateInitialized === undefined ||
!isXMLView(changeEvent.document.uri)
) {
return;
}

await Promise.all([manifestStateInitialized, ui5yamlStateInitialized]);
await Promise.all([
manifestStateInitialized,
ui5yamlStateInitialized,
resourceBundletStateInitialized,
]);
const documentUri = changeEvent.document.uri;
const document = documents.get(documentUri);
if (document !== undefined) {
Expand Down Expand Up @@ -263,11 +281,13 @@ connection.onCodeAction(async (params) => {
version: ui5Model.version,
});

const resourceBundle = getResourceBundleData(documentPath);
const diagnostics = params.context.diagnostics;
const codeActions = diagnosticToCodeActionFix(
textDocument,
diagnostics,
ui5Model
ui5Model,
resourceBundle
);
getLogger().trace("`computed codeActions", { codeActions });
return codeActions;
Expand Down
1 change: 1 addition & 0 deletions packages/language-server/src/swa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export function initSwa(
export const TRACK_EVENTS = {
MANIFEST_STABLE_ID: "manifest stable ID fix",
XML_UI5_DOC_HOVER: "XML UI5 Doc Hover",
MANIFEST_HARDCODED_I18N_STRING: "hardcoded i18n string fix",
};

Object.freeze(TRACK_EVENTS);
Expand Down
5 changes: 5 additions & 0 deletions packages/language-server/src/xml-view-diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ function validationIssuesToLspDiagnostics(
...commonDiagnosticPros,
code: validations.NON_STABLE_ID.code,
};
case "UseOfHardcodedI18nString":
return {
...commonDiagnosticPros,
code: validations.HARDCODED_I18N_STRING.code,
};
case "UseOfDeprecatedClass":
case "UseOfDeprecatedProperty":
case "UseOfDeprecatedEvent":
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<mvc:View xmlns:m="sap.m"
<mvc:View xmlns:m="sap.m"
xmlns:mvc="sap.ui.core.mvc">
<m:Page 🢂navButtonText🢀="dummy-text">
<m:Page 🢂navButtonType🢀="Back">
</m:Page>
<m:App 🢂orientationChange🢀="dummy-text">
</m:App>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
},
"severity": 2,
"source": "UI5 Language Assistant",
"message": "The navButtonText property is deprecated since version 1.20. Deprecated since the MVI theme is removed now. This property only affected the NavButton in that theme.",
"message": "The navButtonType property is deprecated since version 1.20. Deprecated since the MVI theme is removed now. This property is only usable with a Button text in that theme.",
"tags": [2]
},
{
Expand Down
Loading