Skip to content

Fix #268 by introducing a MaybeN type #425

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

Closed
wants to merge 2 commits into from

Conversation

thomasjm
Copy link
Collaborator

Taking a stab at fixing #268 by introducing a new MaybeN type, which is just like Maybe except that Nothing values get encoded to JSON null. This is inspired by the existing List type.

This can be used to conform properly to the LSP spec, which sometimes specifies types like a | null and sometimes a? (i.e. omit the value when absent).

The downside of this approach is that for every field you convert from Maybe to MaybeN, you have to go through all usages of that field and change them. I've included an example of doing this for InitializeParams; for full compliance we would have to do the same for all Maybe fields that are supposed to be a | null.

Of course, it's unpleasant to expose this encoding detail in the field types and make all these changes. This seemed like the most straightforward way, but a cleaner approach might be possible with TH. For example, maybe we could extend makeExtendingDataType so that it takes extra arguments indicating which type of Maybe to use, and generates suitable FromJSON/ToJSON instances manually? FWIW a search through the LSP spec indicates about 40 cases of | null to handle.

@michaelpj
Copy link
Collaborator

I was actually thinking we could tackle this alongside #421. If we completely overhaul the type generation to go off the new metaModel.json, then we can also hopefully fix this issue in a systematic way.

@thomasjm
Copy link
Collaborator Author

Oh wow, that would be nice!

@michaelpj
Copy link
Collaborator

Sadly I don't know when I'm going to get to that :/ So maybe we should do this anyway.

@michaelpj
Copy link
Collaborator

Maybe I'll take a stab at it this weekend. If we're going to force everyone to change how they use the types, we might as well only make them do it once 😆

@thomasjm
Copy link
Collaborator Author

That would be awesome, feel free to hit me up if there's any way I can help.

@michaelpj
Copy link
Collaborator

@thomasjm how big a problem are these issues for you? I'm chipping away at the metamodel generation approach, but it'll take a while.

@thomasjm
Copy link
Collaborator Author

Well, every time I file one of these PRs it's because I've encountered a compliant LSP server in the wild that doesn't work with lsp-types :). I'm trying to work with a number of these.

It's not super urgent to merge these because I just use my own fork, but I thought I'd try to contribute them all back. I'm not sure if anyone else is experiencing LSP compliance pain. If the metamodel thing can bring compliance to 100% in one fell swoop it will be awesome for increasing confidence in this library.

@michaelpj
Copy link
Collaborator

Should be fixed in master

@michaelpj michaelpj closed this Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants