Add missing types from atom-languageclient #57
Conversation
aminya
changed the title
| @@ -13,7 +24,7 @@ export interface CodeActionProvider { | |||
| getCodeActions( | |||
| editor: Atom.TextEditor, | |||
| range: Atom.Range, | |||
| diagnostics: Message[] | |||
| diagnostics: Diagnostic[] // | Message[] | |||
There was a problem hiding this comment.
atom-languageclient uses Diagnostic. Not sure if linter Message supports code action API yet
There was a problem hiding this comment.
Diagnostic is from atom-ide-ui. Which is a slight variation on linter Message. Actually defined here: https://github.com/facebookarchive/atom-ide-ui/blob/master/modules/atom-ide-ui/pkg/atom-ide-diagnostics/lib/types.js#L93
I would suggest we rather scrub this legacy off of atom-languageclient rather than dragging it into the town square, figuratively speaking.
Since we're expecting Linter to handle our diagnostic messages, we really should use the Linter messages and not an ad-hoc approximation defined above.
There was a problem hiding this comment.
I agree. We should fix this issue.
The Message type is not compatible with Diagnostic, and Diagnostic is what is used right now in atom-languageclient. Since it is part of a bigger change and codeAction is not implemented yet in the Linter package), I am going to keep Diagnostic for now, so I can merge the linked PR.
The Diagnostic types were actually removed in this WIP PR, So, we should eventually do the big work, and fix everything.
atom-community/atom-languageclient#112
There was a problem hiding this comment.
More types is probably good. However, there are a few questionable places I think? I've added a few comments. I'll mark this as requesting changes.
The most questionable one is substituting Message for Diagnostic. We really should strive to get closer to Linter spec, and where possible avoid dragging up parts of things Facebook killed a long time ago.
| @@ -13,7 +24,7 @@ export interface CodeActionProvider { | |||
| getCodeActions( | |||
| editor: Atom.TextEditor, | |||
| range: Atom.Range, | |||
| diagnostics: Message[] | |||
| diagnostics: Diagnostic[] // | Message[] | |||
There was a problem hiding this comment.
Diagnostic is from atom-ide-ui. Which is a slight variation on linter Message. Actually defined here: https://github.com/facebookarchive/atom-ide-ui/blob/master/modules/atom-ide-ui/pkg/atom-ide-diagnostics/lib/types.js#L93
I would suggest we rather scrub this legacy off of atom-languageclient rather than dragging it into the town square, figuratively speaking.
Since we're expecting Linter to handle our diagnostic messages, we really should use the Linter messages and not an ad-hoc approximation defined above.
| activeSignature?: number | ||
| activeParameter?: number | ||
| activeSignature?: number | null | ||
| activeParameter?: number | null |
There was a problem hiding this comment.
As far as I can tell, this nulls serve no practical purpose and the only reason they are here is because of flowjs. I suggest we lose them.
There was a problem hiding this comment.
Yeah, but these types were being used with the null union in atom-languageclient. Removing null at this point is dangerous in my opinion.
I compared the types in atom-languageclient with the types we had here in atom-ide-base, and I added the missing ones here.
The types will be removed from atom-languageclient once this PR is merged.
atom-community/atom-languageclient#121
We no longer need to maintain the types in two places.