Highlight #34
Highlight #34
Conversation
src/misc_functions/highlight_util.ts
Outdated
| @@ -0,0 +1,20 @@ | |||
| import { SubAction } from "../types"; | |||
|
|
|||
| export interface HighlightScope { | |||
There was a problem hiding this comment.
This should probably go into src/types.ts
src/misc_functions/highlight_util.ts
Outdated
| export function actionFromScope(scope: HighlightScope): SubAction { | ||
| return { | ||
| data: scope, | ||
| high: scope.end, |
There was a problem hiding this comment.
If start and end are included in the SubAction, why are they in HighlightScope. Wouldn't it be better for this function to be something like:
export function createHighlight(scopes: string[], // Or HighlightInnner if we want to support additional properties
low:number,
high: number
): SubAction {return {type:"highlight",data: scopes, low, high};}
| it("should return the correct highlight data", () => { | ||
| const reader = new StringReader("test"); | ||
| const out = literalArgumentParser.parse(reader, properties); | ||
| assert.deepStrictEqual( |
There was a problem hiding this comment.
Maybe we need a better way of managing testing actions: actions should be tested inside each test, not as a seperate test. Maybe an AssertActions type which takes over numActions in assertReturn. Additionally assertReturn's signature should use the object pattern, because there are two many arguments. If you don't mind, I could do that in this branch.
src/types.ts
Outdated
| export type SubAction = | ||
| | SubActionBase<"hover", string> | ||
| | SubActionBase<"format", string>; | ||
| | SubActionBase<"format", string> | ||
| | SubActionBase<"highlight", HighlightScope>; |
There was a problem hiding this comment.
Would you mind alphabetising these (I know they weren't alphabetical previously)
| ``` | ||
|
|
||
| ## Various scopes | ||
|
|
There was a problem hiding this comment.
The number of scopes here seems very complicated. You might also want to see microsoft/language-server-protocol#124. Having read through the arguments there, I have realised that it's probably better if the server sends highlighting, because we can then send it for each line (This is only better for our server)
There was a problem hiding this comment.
Additionally, we might want to only support a more simple highlighting method initially until microsoft/language-server-protocol#18 is resolved, which seems to be proceeding in microsoft/vscode-languageserver-node#367. We don't want to make a massively complex system which becomes deprecated in a few weeks.
There was a problem hiding this comment.
Better, but still needs some changes
It might even be better not to include a custom highlighting solution, and certainly not a complex one.
i.e. we should emulate the highlighting from vanilla until the upstream changes are passed.
However, we certainly should have a better highlighting system than vanilla eventually.
| scopes: [ | ||
| { | ||
| line: number, | ||
| scopes: string[] |
There was a problem hiding this comment.
So when the client recieves this request, it should highlight each occurance of a line number in every open document in the correct colours for the scopes?
How does that make any sense?
Surely we need to send ranges with scopes
| ``` | ||
|
|
||
| ## Various scopes | ||
|
|
There was a problem hiding this comment.
Additionally, we might want to only support a more simple highlighting method initially until microsoft/language-server-protocol#18 is resolved, which seems to be proceeding in microsoft/vscode-languageserver-node#367. We don't want to make a massively complex system which becomes deprecated in a few weeks.
| @@ -19,6 +19,13 @@ const parser: Parser = { | |||
| if (reader.string.substring(begin, end) === literal) { | |||
| reader.cursor = end; | |||
| if (reader.peek() === " " || !reader.canRead()) { | |||
| helper.addActions( | |||
There was a problem hiding this comment.
This being an action is actually quite awkward, because if there is ambiguity, then what should happen? Text can only be highlighted in one colour.
Additionally, what about in the case of failures. Do we include the mangled scopes. What about if parsing one thing failed with some highlighting, but something else suceeded. How do we prioritise the values.
Maybe helper.merge could manage this if the merge argument fails, but that would make helper.merge O(n) of actions.
| scopes: ["argument", "literal"], | ||
| start: begin | ||
| }) | ||
| ); | ||
| return helper.succeed(); |
There was a problem hiding this comment.
This can be updated to helper.addActions(...).suceed()
No description provided.