Skip to content

Fix "Add missing record fields" code action on v11+ #1088

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

Merged
Merged
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
26 changes: 20 additions & 6 deletions server/src/codeActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export let findCodeActionsInDiagnosticsMessage = ({
diagnosticMessage.forEach((line, index, array) => {
// Because of how actions work, there can only be one per diagnostic. So,
// halt whenever a code action has been found.
let codeActionEtractors = [
let codeActionExtractors = [
simpleTypeMismatches,
didYouMeanAction,
addUndefinedRecordFieldsV10,
Expand All @@ -162,7 +162,7 @@ export let findCodeActionsInDiagnosticsMessage = ({
wrapInSome,
];

for (let extractCodeAction of codeActionEtractors) {
for (let extractCodeAction of codeActionExtractors) {
let didFindAction = false;

try {
Expand Down Expand Up @@ -319,12 +319,14 @@ let handleUndefinedRecordFieldsAction = ({
file,
range,
diagnostic,
todoValue
}: {
recordFieldNames: string[];
codeActions: filesCodeActions;
file: string;
range: p.Range;
diagnostic: p.Diagnostic;
todoValue: string
}) => {
if (recordFieldNames != null) {
codeActions[file] = codeActions[file] || [];
Expand Down Expand Up @@ -373,16 +375,26 @@ let handleUndefinedRecordFieldsAction = ({
newText += paddingContentRecordField;
}

newText += `${fieldName}: failwith("TODO"),\n`;
newText += `${fieldName}: ${todoValue},\n`;
});

// Let's put the end brace back where it was (we still have it to the direct right of us).
newText += `${paddingContentEndBrace}`;
} else {
// A single line record definition body is a bit easier - we'll just add the new fields on the same line.
newText += ", ";

// For an empty record (`range.end.character - range.start.character == 2`),
// we don't want to add an initial trailing comma as that would be invalid syntax.
//
// We assume that records that already contain some characters between
// their braces have at least one field and therefore we need to insert
// an initial trailing comma.
if (range.end.character - range.start.character > 2) {
newText += ", ";
}
Comment on lines +386 to +394
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug fix 2: initial comma was inserted when using this code action on empty records, resulting in invalid syntax


newText += recordFieldNames
.map((fieldName) => `${fieldName}: failwith("TODO")`)
.map((fieldName) => `${fieldName}: ${todoValue}`)
.join(", ");
}

Expand Down Expand Up @@ -440,6 +452,7 @@ let addUndefinedRecordFieldsV10: codeActionExtractor = ({
diagnostic,
file,
range,
todoValue: `failwith("TODO")`
});
}

Expand All @@ -458,7 +471,7 @@ let addUndefinedRecordFieldsV11: codeActionExtractor = ({
if (line.startsWith("Some required record fields are missing:")) {
let theLine = line;
if (theLine.endsWith(".")) {
theLine = theLine.slice(0, theLine.length - 2);
theLine = theLine.slice(0, theLine.length - 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug fix 1: The last character of the last field was cut off

}

let recordFieldNames = theLine
Expand Down Expand Up @@ -486,6 +499,7 @@ let addUndefinedRecordFieldsV11: codeActionExtractor = ({
diagnostic,
file,
range,
todoValue: `%todo`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since %todo was introduced in v11.1.0, I wanted to add a version check in addUndefinedRecordFieldsV11, but passing in project.rescriptVersion from triggerIncrementalCompilationOfFile all the way through to addUndefinedRecordFieldsV11 seemed like it wasn't worth the tradeoff in additional complexity

});
}

Expand Down