-
-
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
Default dialect when no schema specified #25
Default dialect when no schema specified #25
Conversation
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 did a first pass review, but you need to rebase to the latest commits on "main" before I can do a full review.
The new config must not have a default value. The default behavior should be that users need to use $schema
to declare their intended dialect. They need to explicitly set the config to get the "defaultDialect" behavior.
In both demos, you never set the config. You only show what happens when the config's default gets used. Show the behavior without the config set, then set it and show the behavior with the config set.
In the neovim demo, at the end, the language server crashes. You'll need to fix that.
0adf264
to
dbdc205
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.
It's looking good so far. I've left a few (mostly cosmetic) suggestions inline, but I have a couple more things I noticed.
I haven't figured out how best to organize the code yet. There's not a lot of organization, but there is some. I'm trying to keep related functionality grouped as well as possible. You'll see each feature headed by a comment in all caps marking the section where that feature is implemented. The exception to this grouping is initialization stuff because that all has to be in the same place. I'd like to see a section for // CONFIGURATION
with all the configuration-related code (other than initialization) under that heading. That would help keep things somewhat organized until I can come up with a better organization strategy.
I did find one bug. The semantic tokens functionality isn't working when the defaultDialect is used. Oddly, I'm not seeing semantic tokens working at all in the vscode demo, but you can see what I'm talking about in the neovim demo. The keywords are highlighted when $schema
is used, but that disappears when $schema
is removed and the defaultDialect applies. This should be simple to fix, just use the config in the semantic tokens functionality the same way you use it for the schema validation functionality.
The sematic tokens functionality seems to be theme dependent, as mentioned here too https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#:~:text=Semantic%20tokenization%20allows%20language%20servers,the%20syntax%20highlighting%20from%20grammars. |
From your screenshot, it looks like you have a theme that supports semantic tokens now. Since the declared dialect ( |
I've updated the recording of how the highlighting works now after updating the code. We tried other ways too by simply doing it the way
I've tried putting all the configuration code that we've added under one section also keeping in mind how the other sections are organised , let me know if this structure doesn't work or if it should be some other way |
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.
It looks like this and this haven't been addressed yet. Other than that, the code looks good.
I notice in the vscode that you're getting a lot of errors pop up. Is that related to what you were talking about here? The highlighting seemed to be working fine otherwise. I could be wrong, but I vaguely remember that the semantic tokens handler couldn't take an async function. That could be related to the problem you're facing.
In the neovim demo, you never set the config to show the behavior when the config is set. You're only showing me the existing functionality, not the new functionality.
@jdesrosiers |
The demos look good! It looks like your branch is out of date and has some conflicts. Please rebase and resolve conflicts before I do my next review. |
Co-authored-by: Rajat <rajatkhanduri290102@gmail.com> Co-authored-by: Diya <diya.solanki.31@gmail.com>
Co-authored-by: Rajat <rajatkhanduri290102@gmail.com> Co-authored-by: Diya <diya.solanki.31@gmail.com>
53c47a4
to
e101e78
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.
Almost there. I left a couple minor suggestions inline, but I found one bug in my exploratory testing.
If you set defaultDialect
to an unsupported dialect (For example: https://example.com/my-dialect
) and the schema doesn't have $schema
, the error you get the "no dialect" error instead of the "unknown dialect" error.
…of global settings
|
53ceb46
to
d6b9d03
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.
🎉
I merged manually because I wanted to squash commits, but it seems to have confused Github. I have to close this PR, but it was actually successfully merged. Thanks for your hard work! |
Resolves #6 #4
Changes:
$schema
keyword is used.Video demonstrating the changes:
VS Code :
vscodedemosemantic.mp4
Neovim :
e94dae4e-8b3b-4b62-a5d7-f261bcf35e0d.mp4
Thanks to my partner, @wthrajat , for his invaluable assistance throughout our collaboration. His unwavering support, dedication, and assistance in code debugging, guidance in Neovim testing, have significantly enhanced our workflow efficiency and productivity.
This PR has been created as a part of GSoC qualification task.