-
Notifications
You must be signed in to change notification settings - Fork 59
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
Fix "Add missing record fields" code action on v11+ #1088
Conversation
This reverts commit 2265806.
@@ -486,6 +499,7 @@ let addUndefinedRecordFieldsV11: codeActionExtractor = ({ | |||
diagnostic, | |||
file, | |||
range, | |||
todoValue: `%todo` |
There was a problem hiding this comment.
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
@@ -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); |
There was a problem hiding this comment.
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
// 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 += ", "; | ||
} |
There was a problem hiding this comment.
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
Fantastic work @mediremi, thank you for this and the great work you've been doing recently! How can I get in touch with you? We'd like to invite you to our contributor Discord, if you're interested in joining. |
I'd definitely be interested in joining the Discord! My email is [email protected] and my discord username is mediremi :) |
This PR fixes two bugs in the "Add missing record fields" code action:
.slice(0, length - 2)
that should have beenlength - 1
I've also changed the v11 version of this code action to use
%todo
instead offailwith("TODO")
.