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: quickFix for enum, const, property #900

Open
wants to merge 5 commits into
base: main
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: 3 additions & 0 deletions src/languageservice/parser/jsonParser07.ts
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,7 @@ function validate(
),
source: getSchemaSource(schema, originalSchema),
schemaUri: getSchemaUri(schema, originalSchema),
data: { values: schema.enum },
});
}
}
Expand All @@ -907,6 +908,7 @@ function validate(
source: getSchemaSource(schema, originalSchema),
schemaUri: getSchemaUri(schema, originalSchema),
problemArgs: [JSON.stringify(schema.const)],
data: { values: [schema.const] },
});
validationResult.enumValueMatch = false;
} else {
Expand Down Expand Up @@ -1385,6 +1387,7 @@ function validate(
length: propertyNode.keyNode.length,
},
severity: DiagnosticSeverity.Warning,
code: ErrorCode.PropertyExpected,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if it wouldn't be better to unify data.properties to data.values for the incorrect property

message: schema.errorMessage || localize('DisallowedExtraPropWarning', MSG_PROPERTY_NOT_ALLOWED, propertyName),
source: getSchemaSource(schema, originalSchema),
schemaUri: getSchemaUri(schema, originalSchema),
Expand Down
57 changes: 52 additions & 5 deletions src/languageservice/services/yamlCodeActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@ import { FlowStyleRewriter } from '../utils/flow-style-rewriter';
import { ASTNode } from '../jsonASTTypes';
import * as _ from 'lodash';
import { SourceToken } from 'yaml/dist/parse/cst';
import { ErrorCode } from 'vscode-json-languageservice';

interface YamlDiagnosticData {
schemaUri: string[];
values?: string[];
properties?: string[];
}
export class YamlCodeActions {
private indentation = ' ';
Expand All @@ -54,6 +57,7 @@ export class YamlCodeActions {
result.push(...this.getUnusedAnchorsDelete(params.context.diagnostics, document));
result.push(...this.getConvertToBlockStyleActions(params.context.diagnostics, document));
result.push(...this.getKeyOrderActions(params.context.diagnostics, document));
result.push(...this.getQuickFixForPropertyOrValueMismatch(params.context.diagnostics, document));

return result;
}
Expand Down Expand Up @@ -221,7 +225,7 @@ export class YamlCodeActions {
const results: CodeAction[] = [];
for (const diagnostic of diagnostics) {
if (diagnostic.code === 'flowMap' || diagnostic.code === 'flowSeq') {
const node = getNodeforDiagnostic(document, diagnostic);
const node = getNodeForDiagnostic(document, diagnostic);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just typo

if (isMap(node.internalNode) || isSeq(node.internalNode)) {
const blockTypeDescription = isMap(node.internalNode) ? 'map' : 'sequence';
const rewriter = new FlowStyleRewriter(this.indentation);
Expand All @@ -242,7 +246,7 @@ export class YamlCodeActions {
const results: CodeAction[] = [];
for (const diagnostic of diagnostics) {
if (diagnostic?.code === 'mapKeyOrder') {
let node = getNodeforDiagnostic(document, diagnostic);
let node = getNodeForDiagnostic(document, diagnostic);
while (node && node.type !== 'object') {
node = node.parent;
}
Expand Down Expand Up @@ -292,8 +296,8 @@ export class YamlCodeActions {
item.value.end.splice(newLineIndex, 1);
}
} else if (item.value?.type === 'block-scalar') {
const nwline = item.value.props.find((p) => p.type === 'newline');
if (!nwline) {
const newline = item.value.props.find((p) => p.type === 'newline');
if (!newline) {
item.value.props.push({ type: 'newline', indent: 0, offset: item.value.offset, source: '\n' } as SourceToken);
}
}
Expand All @@ -312,9 +316,52 @@ export class YamlCodeActions {
}
return results;
}

/**
* Check if diagnostic contains info for quick fix
* Supports Enum/Const/Property mismatch
*/
private getPossibleQuickFixValues(diagnostic: Diagnostic): string[] | undefined {
if (typeof diagnostic.data !== 'object') {
return;
}
if (
diagnostic.code === ErrorCode.EnumValueMismatch &&
'values' in diagnostic.data &&
Array.isArray((diagnostic.data as YamlDiagnosticData).values)
) {
return (diagnostic.data as YamlDiagnosticData).values;
} else if (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this else really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean quickFix for properties? or can you explain your point further?

  • I think that property quickFix can be useful
  • note that there are different property names in diagnostic.data object (values vs properties)

diagnostic.code === ErrorCode.PropertyExpected &&
'properties' in diagnostic.data &&
Array.isArray((diagnostic.data as YamlDiagnosticData).properties)
) {
return (diagnostic.data as YamlDiagnosticData).properties;
}
}

private getQuickFixForPropertyOrValueMismatch(diagnostics: Diagnostic[], document: TextDocument): CodeAction[] {
const results: CodeAction[] = [];
for (const diagnostic of diagnostics) {
const values = this.getPossibleQuickFixValues(diagnostic);
if (!values?.length) {
continue;
}
for (const value of values) {
results.push(
CodeAction.create(
value,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns only the value which is not very descriptive of the operation. perhaps we can replace it with something like

Suggested change
value,
`Use value "${value}"`,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have preferences here, I can remember that I tried a few combinations but I ended up with only {value}.
I was inspired by the English spelling checker extension (Code Spell Checker extension)
image

small disadvantage of Use value "${value}" is that this PR adds quickfix also for properties of the object (not value), so we need to display the text based on the error type

  • but for sure it's not a problem

so please make a decision

createWorkspaceEdit(document.uri, [TextEdit.replace(diagnostic.range, value)]),
CodeActionKind.QuickFix
)
);
}
}
return results;
}
}

function getNodeforDiagnostic(document: TextDocument, diagnostic: Diagnostic): ASTNode {
function getNodeForDiagnostic(document: TextDocument, diagnostic: Diagnostic): ASTNode {
const yamlDocuments = yamlDocumentsCache.getYamlDocument(document);
const startOffset = document.offsetAt(diagnostic.range.start);
const yamlDoc = matchOffsetToDocument(startOffset, yamlDocuments);
Expand Down
7 changes: 5 additions & 2 deletions test/schemaValidation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { KUBERNETES_SCHEMA_URL } from '../src/languageservice/utils/schemaUrls';
import { IProblem } from '../src/languageservice/parser/jsonParser07';
import { JSONSchema } from '../src/languageservice/jsonSchema';
import { TestTelemetry } from './utils/testsTypes';
import { ErrorCode } from 'vscode-json-languageservice';

describe('Validation Tests', () => {
let languageSettingsSetup: ServiceSetup;
Expand Down Expand Up @@ -396,7 +397,8 @@ describe('Validation Tests', () => {
4,
DiagnosticSeverity.Error,
`yaml-schema: file:///${SCHEMA_ID}`,
`file:///${SCHEMA_ID}`
`file:///${SCHEMA_ID}`,
ErrorCode.PropertyExpected
)
);
})
Expand Down Expand Up @@ -1289,7 +1291,7 @@ obj:
4,
18,
DiagnosticSeverity.Error,
'yaml-schema: Package',
'yaml-schema: Composer Package',
'https://raw.githubusercontent.com/composer/composer/master/res/composer-schema.json'
)
);
Expand All @@ -1312,6 +1314,7 @@ obj:
DiagnosticSeverity.Error,
'yaml-schema: Drone CI configuration file',
'https://json.schemastore.org/drone',
ErrorCode.PropertyExpected,
{
properties: [
'type',
Expand Down
12 changes: 11 additions & 1 deletion test/utils/verifyError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,19 @@ export function createDiagnosticWithData(
severity: DiagnosticSeverity = 1,
source = 'YAML',
schemaUri: string | string[],
code: string | number = ErrorCode.Undefined,
data: Record<string, unknown> = {}
): Diagnostic {
const diagnostic: Diagnostic = createExpectedError(message, startLine, startCharacter, endLine, endCharacter, severity, source);
const diagnostic: Diagnostic = createExpectedError(
message,
startLine,
startCharacter,
endLine,
endCharacter,
severity,
source,
code
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just code added to parameters

);
diagnostic.data = { schemaUri: typeof schemaUri === 'string' ? [schemaUri] : schemaUri, ...data };
return diagnostic;
}
Expand Down
55 changes: 55 additions & 0 deletions test/yamlCodeActions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { setupTextDocument, TEST_URI } from './utils/testHelper';
import { createDiagnosticWithData, createExpectedError, createUnusedAnchorDiagnostic } from './utils/verifyError';
import { YamlCommands } from '../src/commands';
import { LanguageSettings } from '../src';
import { ErrorCode } from 'vscode-json-languageservice';

const expect = chai.expect;
chai.use(sinonChai);
Expand Down Expand Up @@ -377,4 +378,58 @@ animals: [dog , cat , mouse] `;
]);
});
});

describe('Enum value or property mismatch quick fix', () => {
it('should generate proper action for enum mismatch', () => {
const doc = setupTextDocument('foo: value1');
const diagnostic = createDiagnosticWithData(
'message',
0,
5,
0,
11,
DiagnosticSeverity.Hint,
'YAML',
'schemaUri',
ErrorCode.EnumValueMismatch,
{ values: ['valueX', 'valueY'] }
);
const params: CodeActionParams = {
context: CodeActionContext.create([diagnostic]),
range: undefined,
textDocument: TextDocumentIdentifier.create(TEST_URI),
};
const actions = new YamlCodeActions(clientCapabilities);
const result = actions.getCodeAction(doc, params);
expect(result.map((r) => r.title)).deep.equal(['valueX', 'valueY']);
expect(result[0].edit.changes[TEST_URI]).deep.equal([TextEdit.replace(Range.create(0, 5, 0, 11), 'valueX')]);
});

it('should generate proper action for wrong property', () => {
const doc = setupTextDocument('foo: value1');
const diagnostic = createDiagnosticWithData(
'message',
0,
0,
0,
3,
DiagnosticSeverity.Hint,
'YAML',
'schemaUri',
ErrorCode.PropertyExpected,
{
properties: ['fooX', 'fooY'],
}
);
const params: CodeActionParams = {
context: CodeActionContext.create([diagnostic]),
range: undefined,
textDocument: TextDocumentIdentifier.create(TEST_URI),
};
const actions = new YamlCodeActions(clientCapabilities);
const result = actions.getCodeAction(doc, params);
expect(result.map((r) => r.title)).deep.equal(['fooX', 'fooY']);
expect(result[0].edit.changes[TEST_URI]).deep.equal([TextEdit.replace(Range.create(0, 0, 0, 3), 'fooX')]);
});
});
});
Loading