-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Server - Code completion for $schema #27
Conversation
4339921
to
d4a96db
Compare
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.
There's so much unnecessary formatting changes, I can barely find your actual changes. I suggest you disable prettier or what every auto formatting you're using. EsLint is configured to provide all the code style guidance you need for this project. Please undo all the formatting changes.
bfa69d5
to
2e279d3
Compare
language-server/src/server.js
Outdated
@@ -32,6 +33,7 @@ setMetaSchemaOutputFormat(DETAILED); | |||
setShouldValidateSchema(false); | |||
|
|||
const isSchema = RegExp.prototype.test.bind(/(?:\.|\/|^)schema\.json$/); | |||
const availableSchemas = getDialectIds(); |
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.
This can't be global. If someone adds a custom meta-schema in their workspace, that needs to show up in the list as well. I hadn't thought of this until now, but if someone removes their custom meta-schema from their workspace, it should be removed from the list. We might need to add another function in @hyperjump/json-schema
to remove a dialect that can be called when a custom meta-schema is removed from the workspace.
language-server/src/server.js
Outdated
function findPropertyAtPosition(instance, position) { | ||
for (const [key, value] of instance.entries()) { | ||
if ( | ||
position.line >= key.startPosition().line | ||
&& position.character >= key.startPosition().character | ||
&& position.line <= value.endPosition().line | ||
&& position.character <= value.endPosition().character | ||
) { | ||
return { key, value }; | ||
} | ||
} | ||
return undefined; | ||
} |
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.
I think this would be useful as a method in JsoncInstance instead of as a function by itself.
Can you use findNodeAtOffset
in jsonc-parser to avoid some work here?
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.
Yeah, we can avoid some work in the method by using findNodeAtOffset
.
2e279d3
to
e998d4c
Compare
findProperty(instance) { | ||
const keyNode = findNodeAtOffset(instance.root, instance.node.children[0]?.children[0]?.offset); | ||
const valNode = findNodeAtOffset(instance.root, instance.node.children[0]?.children[1]?.offset); | ||
return { key: keyNode, value: valNode }; | ||
} |
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.
This doesn't make sense. You create an instance and then pass it to itself? And at no point do you actually pass the position from the onCompletion event to this method. You need to use that position to find the offset (hint: this.textDocument.???
). Then you can use that offset to get the node with findNodeAtOffset
. Then use that node to create and return a JsoncInstance representing that position. To build the JsoncInstance, you need to build the JSON Pointer for that location as well (hint: getNodePath
from jsonc-parser). You don't need to return the key because you'll have all the information you need from the pointer in the JsoncInstance.
Given that my suggestion changes what this method does and its inputs and outputs, it's going to need a better name that accurately describes what it does.
78e5f12
to
eba5da0
Compare
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.
The code is looking great! But, there's something not quite right. In neovim, the suggestions work great, but I get an error when I select one of the options.
Completion works in vscode, but if I have an existing schema,
{
"$schema": "http://json-schema.org/draft-07#",
"type": "string"
}
and I change the dialect,
{
"$schema": "draft2020",
"type": "string"
}
when I select the suggested dialect, the the comma at the end of the line gets removed when the suggestion is inserted.
{
"$schema": "https://json-schema.org/draft/2020-12/schema"
"type": "string"
}
UPDATE: The neovim error only occurs in the above situation.
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.
Looks good! I'm excited to have this feature.
Please squash commits and then I'll merge.
54bf9c1
to
6ec162e
Compare
Resolves #9
Tested on both neovim and vscode
VSCode:
Neovim: