atom-community / atom-languageclient

Provide integration support for adding Language Server Protocol servers to Atom.
https://www.npmjs.com/package/atom-languageclient
MIT License
45 stars 13 forks source link

Added null check to some adapters and changed type definition #150

Closed ayame113 closed 3 years ago

ayame113 commented 3 years ago

Some LSP requests lack null checking and are causing errors, so I fixed them.

image

I made the following changes:

  1. Added null check (https://github.com/atom-community/atom-languageclient/commit/e20bba5a4fe06b4f1a24f2063b1a60fcd5da1514)
  2. Fixed the type definition (https://github.com/atom-community/atom-languageclient/commit/85cc505a5c9999e4da044996aa49e436d32d017a)
  3. Use the vscode-languageserver-protocol package for type definition (https://github.com/atom-community/atom-languageclient/compare/85cc505..d665159)

If you don't need the third change, reverting it will still work.

UziTech commented 3 years ago

I think we were discussing this in https://github.com/atom-community/atom-languageclient/pull/132#discussion_r595580845

I'm not sure if it is better to throw an error if the lsp returns something invalid or fail silently like this PR. Is null or undefined a valid return for .codeAction?

aminya commented 3 years ago

I think we were discussing this in #132 (comment)

This is different though.

UziTech commented 3 years ago

This is different though.

How? Why are these checks necessary if null or undefined are not correct outputs of .codeAction anyway?

If null or undefined are valid outputs then we should change the types to reflect that.

aminya commented 3 years ago

I meant the changes under lib/languageclient.ts which seem reasonable as they are using the original types.

UziTech commented 3 years ago

I meant the changes under lib/languageclient.ts which seem reasonable as they are using the original types.

Ya I think those changes are good. I'm just not sure about null checks.

ayame113 commented 3 years ago

(some comment)

aminya commented 3 years ago

I'll review today or tomorrow

github-actions[bot] commented 3 years ago

:tada: This PR is included in version 1.8.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: