-
Notifications
You must be signed in to change notification settings - Fork 177
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
Allow loading of custom data #142
Conversation
src/cssLanguageService.ts
Outdated
|
||
export type Stylesheet = {}; | ||
export * from './cssLanguageTypes'; | ||
export * from 'vscode-languageserver-types'; | ||
|
||
export interface LanguageServiceOptions { |
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.
move to 'types'...
|
||
export function getCSSLanguageService(): LanguageService { | ||
export function getCSSLanguageService(options?: LanguageServiceOptions): LanguageService { | ||
handleCustomData(options); |
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.
If we choose this API, we must store the properies/directives per LanguageService, not in global arrays. Users can create any number of language services, each with a different set of data.
I would suggest to make a new type CSSPropertySet
(name to be debated) containing properties/directives/..., that is registered globaly with the vscode-css-languageservice
(exported const propertySets : CSSPropertySet[]
) Our built-in properties (generated from MDN) should also be represented in one (or multiple) CSSPropertySets.
In a first step, all language services just use that global list of propertySets. In a later step we allow to create a language service with a custom list of Sets. That language service would then only use that set. It would be good to already prepare our code to avoid accessing CSSPropertySet globaly.
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.
Yeah I was about to check with you. The same problem with HTML now. Do we make the language services stateful? I'll go with stateful then.
src/services/cssCompletion.ts
Outdated
@@ -581,7 +581,10 @@ export class CSSCompletion { | |||
} | |||
|
|||
public getCompletionForTopLevel(result: CompletionList): CompletionList { | |||
for (let entry of languageFacts.getAtDirectives()) { | |||
const atDirectives = languageFacts.getAtDirectives(); |
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 find the original code more readable.
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 need atDirectives[entryName]
for accessing the each entry. It previously returns an array, so let...of
works, now it's an object.
src/services/languageFacts/index.ts
Outdated
import { addPseudoClasses } from './pseudoClasses'; | ||
import { addPseudoElements } from './pseudoElements'; | ||
|
||
let { properties, atdirectives, pseudoclasses, pseudoelements } = browsers.data.css; |
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.
Let's avoid globals at more than one place.
} | ||
} | ||
|
||
const propertySet: { [key: string]: IEntry } = {}; |
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.
Let's avoid globals at more than one point...
return getCSSLanguageService(customData); | ||
} | ||
|
||
function assertCompletion(completions: CompletionList, expected: ItemDescription, document: TextDocument) { |
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'd rather not copy, but just add new tests to completions.tests
For microsoft/vscode#64164.
languageFacts