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

Implement missing 3.17 api #362

Merged
merged 3 commits into from
May 30, 2022
Merged

Implement missing 3.17 api #362

merged 3 commits into from
May 30, 2022

Conversation

CGNonofr
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@kaisalmen kaisalmen left a comment

Choose a reason for hiding this comment

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

Looks good. Do you expect more missing api adaptations? Should we check more carefully before releasing a new version (beta or rc before a major release/change)?

@CGNonofr
Copy link
Collaborator Author

CGNonofr commented May 26, 2022

Looks good. Do you expect more missing api adaptations? Should we check more carefully before releasing a new version (beta or rc before a major release/change)?

That was my bad, I wasn't attentive enough, I just looked at the code and didn't see it at first.

I took a look at the changelog to identify the missing parts:

  • Add support for a completion item label details: in the PR
  • Add support for workspace symbol resolve request: In this PR but useless as not supported by monaco.
  • Add support for shared values on CompletionItemList: in the PR
  • collapsed text in folding is not supported by vscode itself
  • add support for trigger kinds on code action requests: not supported by monaco for the moment
  • Add support for type hierarchies: not supported by monaco
  • Add support for inlay hints: in previous PR
  • Add support for inline values: Not supported by monaco yet
  • Add support for notebook documents: Not supported by monaco
  • Add support for diagnostic pull model: probably nothing to be done but hard to test without any implementation yet
  • Add support for relative patterns in file watchers: nothing do be done in the lib code
  • Everything else do not require any change here

Btw, this file is VERY hard to maintain, I tried to be close as possible to the vscode/vscode-languageclient code but it will never be exactly the same, I don't know if there is any better way to be less hacky. Also, it's very hard to test because there is a lot of possible values/use cases and no implementations exist when the new protocol version just got released)

Btw2, we only support a subset of the features declared supported by the BaseLanguageClient, we should probably disable some feature we don't support, I'll have a look at it)

@kaisalmen
Copy link
Collaborator

@CGNonofr sorry, I was out of town/AFK for a couple of days (longer weekend).

Btw, this file is VERY hard to maintain, I tried to be close as possible to the vscode/vscode-languageclient code but it will never be exactly the same, I don't know if there is any better way to be less hacky. Also, it's very hard to test because there is a lot of possible values/use cases and no implementations exist when the new protocol version just got released)

Yes, I see the problem and we don't have any unit tests and I don't see how we could establish those easily (it is a huge undertaking time wise). Plus, test don't necessarily help at this specific point, because you need to write them, too. They mostly help afterwards to verify the implementation / usage of the API afterwards.

We could establish a review plan, e.g. a markdown file listing what to check (change logs, specific files, etc.) and we both do this before merging. This is also helpful for me to understand what you reviewed / how you did this.

@CGNonofr
Copy link
Collaborator Author

We could establish a review plan, e.g. a markdown file listing what to check (change logs, specific files, etc.) and we both do this before merging. This is also helpful for me to understand what you reviewed / how you did this.

Good idea! We should probably write this somewhere (contributing.md)? because we'll forgot about it when a new LSP version will come out (probably in some years)

@CGNonofr CGNonofr merged commit 402e8e6 into main May 30, 2022
@CGNonofr CGNonofr deleted the implement-missing-3.17-api branch May 30, 2022 08:03
@kaisalmen
Copy link
Collaborator

new LSP version will come out (probably in some years)

Yes, and we could expand this to other things as well, like new monaco-editor version is released, check this and that ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants