-
Notifications
You must be signed in to change notification settings - Fork 28.7k
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
add NotebookFilter
and make it a real thing
#144826
Conversation
Ready for review/fyi but some doc polish is still needed |
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.
On little question: notebook: *
matches every notebook. I tested it. Just checking that this is on purpose (e.g. '*' is a valid value for a notebook type)
@@ -2056,6 +2079,9 @@ declare module 'vscode' { | |||
* | |||
* @example <caption>A language filter that applies to all package.json paths</caption> | |||
* { language: 'json', pattern: '**/package.json' } | |||
* | |||
* @example <caption>A language filter that applies to all python files on jupyter notebooks</caption> | |||
* { language: 'python', notebook: 'jupyter-notebook' } | |||
*/ | |||
export interface DocumentFilter { |
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'm still confused on how DocumentFilter
interacts with NotebookFilter
. I'd sort of expect the new type to be DocumentFilter | NotebookFilter
instead of adding DocumentFilter.notebook
property
Are there use cases where I need to set different properties on an outer DocumentFilter
than are set on the inner NotebookFilter
?
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.
In LSP I did DocumentFilter = TextDocumentFilter | NotebookDocumentFilter where the old DocumentFilter = TextDocumentFilter.
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.
Actually the above statement is incorrect :-).
What we actually want to support is the following: register a hover provider for cell's inside a notebook where the cell's language is python
the notebook document itself is of type jupyter-notebook
and the notebook is stored on disk (e.g. it's scheme is file
).
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.
In LSP I have the following definition
DocumentFilter | { language?: string; notebook?: string | NotebookFilter }
This PR refines #141143 and adds the ability to match not just notebookType but also notebook scheme and pattern.
fyi @dbaeumer