notebookControllerAffinityHidden proposal to allow for hiding of notebook controllers per document#161145
Conversation
…rollers from the picker per document
|
|
||
| declare module 'vscode' { | ||
| // https://github.com/microsoft/vscode/issues/161144 | ||
| export enum NotebookControllerAffinityExtended { |
There was a problem hiding this comment.
I was a slight bit unsure on the best way to just extend an enum, didn't see an easy "typescripty" way to add in the new value. So I went with just adding a new extended type and updating the controller function to take either type. Would love to know if there is a better way to do this.
There was a problem hiding this comment.
This is fair - there are limits to what we can do with TypeScript. On a nit-note: in such cases we usually use the 2 suffix, not Extended, so NotebookControllerAffinity2 might be a bit better
| suggestions.push(all[0]); | ||
| } | ||
| return { all, selected, suggestions }; | ||
| return { all, selected, suggestions, hidden }; |
There was a problem hiding this comment.
Instead of filtering all at this point it seemed to match the current usage to keep all as all, then just provide hidden to indicate the kernels marked as hidden.
jrieken
left a comment
jrieken
left a comment
There was a problem hiding this comment.
looking good at first sight, make sure to either enforce the proposal tho (or fast track to finalization directly)
|
Updates: Also added in the proposed API guard. The guard looked a tiny bit different since I was only adding on an enum value, so instead of guarding the function I just guarded against the new enum value. Tested (extension using the new Hidden value wo/ specifying the proposedAPI): Tested (extension not using the new Hidden value wo/ specifying the proposedAPI, to make sure I'm not erroring for someone using the normal API): Tested (extension using the new Hidden value and specifying the proposedAPI): All looks good from my understanding. Only error thrown when trying to pass in the Hidden value wo/ specifying the API. |
jrieken
left a comment
jrieken
left a comment
There was a problem hiding this comment.
Looking good.
Going forward with the API we need
- jsdoc for all the things
- reconsider the
-1value, maybe one in sequence is fine too - ensure the name Hidden is "good"
3 participants
#161144