-
Notifications
You must be signed in to change notification settings - Fork 70
Conversation
* All rights reserved. | ||
* | ||
* This source code is licensed under the license found in the | ||
* LICENSE file in the root directory of this source tree. |
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 have specified the wrong licence I think. graphql/codemirror-graphql
is under BSD, which requires redistributions (including those with modifications) to follow the same.
Thoughts?
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.
We should change all license info to MIT (I believe we had a very interesting series of events because of this 😃). I haven't changed licensing info for graphql-language-service
and codemirror-graphql
just yet.
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.
That would be a worthwhile effort.
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 completely okay to have codemirror-graphql
export info.js
and reuse that instead of porting the file here. Would that be a more suitable approach?
@@ -46,6 +46,8 @@ import { | |||
|
|||
import {parse, print} from 'graphql'; | |||
import {getAutocompleteSuggestions} from './getAutocompleteSuggestions'; | |||
import {getHoverInformation} from './getHoverInformation'; | |||
import type {Hover} from 'vscode-languageserver-types'; |
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 don't have a good lint rule set up for this, but I'd encourage including type imports on the top of the list of imports.
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.
Done
filePath: Uri, | ||
): Promise<Hover.contents> { | ||
const projectConfig = this._graphQLConfig.getConfigForFile(filePath); | ||
const schema = await this._graphQLCache |
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.
Same comment as the last PR here - could we defer fetching schema until we're absolutely in need of one?
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 is about as late as we can leave it – at this point the MessageProcessor has already validated that the hover position contains a graphql document
* All rights reserved. | ||
* | ||
* This source code is licensed under the license found in the | ||
* LICENSE file in the root directory of this source tree. |
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.
We should change all license info to MIT (I believe we had a very interesting series of events because of this 😃). I haven't changed licensing info for graphql-language-service
and codemirror-graphql
just yet.
Display field type & description information (logic ported from graphql/codemirror-graphql)
I'll investigate consolidating hover rendering logic between this repo and codemirror-graphql separately to this change. I think it would make sense for canonical logic to live in this repo. |
Display field type & description information (logic ported from
graphql/codemirror-graphql)
Fixes: #216