Add notebook support. #268
Conversation
|
Closes #269. |
src/notebook.rs
Outdated
| #[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)] | ||
| #[serde(untagged)] | ||
| pub enum NotebookDocumentFilter { | ||
| ByType(NotebookDocumentFilterByType), | ||
| ByScheme(NotebookDocumentFilterByScheme), | ||
| ByPattern(NotebookDocumentFilterByPattern), | ||
| } |
There was a problem hiding this comment.
The specification defines NotebookDocumentFilter basically as a struct with three optional fields but one of them needs to be set. In the past the specification has "hidden" types like this, by specifying the type as a struct with optional fields and noting in the text, that one field must be set for it to be valid. For example DocumentFilter has the following note:
Please note that for a document filter to be valid at least one of the properties for language, scheme, or pattern must be set. To keep the type definition simple all properties are marked as optional.
For these other cases lsp-types uses structs with optional fields. It would be more consistent and usable if we did the same for NotebookDocumentFilter and NotebookSelector.
There was a problem hiding this comment.
Mm, I see where you're coming from, but I'm not entirely convinced that this should simply use optional fields. You might be correct in that optional fields might be more usable than the other, but more so for those well versed with the spec.
I still think one should try to leverage the type system as much as possible when it comes to defining the domain. The current solution is one way of doing so. Silent errors and unpleasant surprises are pretty easy to introduce if not.
I'm nonetheless open to changing this as suggested, but I think I would like to receive some input from others before deciding on anything. (If that's even up to me.) 😊
There was a problem hiding this comment.
I agree with you, but think that the interface of NotebookDocumentFilter should be consistent with DocumentFilter, because they are basically the same thing. So we should either change this PR or change the interface of DocumentFilter.
Maybe @Marwes or @ebkalderon can help us decide.
There was a problem hiding this comment.
I feel like the patch as is captures the specification better than having 3 optional fields.
The only thing I would consider is since the spec doesn't define the NotebookDocumentFilterBy* types,
you could consider inlining those structures into the enum variants themselves like:
enum NotebookDocumentFilter {
ByType{notebook_type: String, scheme: Option<String>, pattern: Option<String>},
...
}
I feel like the DocumentFilter case is different, since there the spec is actually defining it as a single struct with 3 optional fields, but NetworkDocumentFilter defines it differently, as one of any 3 structs each with 1 required field, and 2 optional fields... enum captures that latter definition better than struct.
I'm overall not very opinionated on whether its worthwhile to eliminate these intermediate types,
and while I do use the library, I haven't really contributed to this library before so my opinion shouldn't really hold much weight honestly.
There was a problem hiding this comment.
I can totally get behind inlining the variants! Great idea :)
There was a problem hiding this comment.
I like how df3eeea captures things, as I also agree that the spec itself differs from how it presents NotebookDocumentFilter and DocumentFilter 🤗
There was a problem hiding this comment.
Inlining it seems like the right trade-off for now. Ideally the LSP spec should homogenize how it defines these two types though (and we should as well), but for now having them be different seems fine, we might be able to find out which is preferred by hearing from people who use both.
Thanks for the review. I went ahead and resolved all the rename suggestions. |
|
I did not notice it earlier, but all fields in the new structs should be pub, otherwise they cannot be set. |
|
Omg 🙈 of course, thanks. I suspect the reason for this sloppy implementation is that I just needed the structs to exist when I was experimenting with enum based lsp services (over dyn dispatch), my apologies. |
|
Hey, Bartek from the Deno team here 🖐️ This patch is quite important to us and blocking us on adding LSP support for Jupyter notebooks. Is there anything we can do to help and expedite landing of this PR? |
|
Hi, this is Jane from the Astral team 👋 Adding on to what @bartlomieju said, this is an important PR that's blocking support for Jupyter notebooks in our new language server. We plan on using this PR (or a potential fork of it) as a git dependency until this gets merged. If there's anything we can do to help get this PR merged, please let us know. |
|
any update |
|
Released as 0.96. Sorry for losing track of this. |
Will conflict with #266 depending on which gets merged first.Depends on #266.