-
Notifications
You must be signed in to change notification settings - Fork 131
Requests: Refactor implIntf and inferIntf parameters #1501
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
base: master
Are you sure you want to change the base?
Requests: Refactor implIntf and inferIntf parameters #1501
Conversation
Fixes #1330 |
Currently implIntf and inferIntf accept a DocumentUri while typedHoles accepts ``` { uri: DocumentUri } ``` When looking at how vscode-ocaml-platform handles it: - For implIntf and inferIntf: ``` let source_uri = Uri.toString (TextDocument.uri document) () in ``` - For typedHoles ``` let uri = TextDocument.uri doc in ``` I don't see any reason to not have the three requests have the same type of parameter
d6de128
to
7bb7fb9
Compare
That's quite right, but is that a reason sufficient-enough to introduce a breaking change in the |
Hi @voodoos ;-) It's not yet widely used and it may introduce some technical debt once people start adding new custom requests picking code from implIntf or typedHoles. Having to already play with inconsistencies when there are only 3 different requests justifies a change before we have to deal with way more, in my opinion :-) |
There is a way to ease the transition from the current API to the new one. A new experimental server capability like, for example, experimental.ocamllsp.handleSwitchImplAndIntf (with 'And') could let the LSP clients know to use the new API. Clients relying on the old API search for experimental.ocamllsp.handleSwitchImplIntf (without 'And'), and therefore they could gracefully fall back to a more basic operation. |
I like this idea @nemethf, thanks for it! I'll improve my PR tomorrow! |
That cost is not high enough to justify breaking older clients or start a complex migration process. There is nothing wrong in the current queries behavior. They don't enforce the same style practices and that's a shame. I agree that we should decide on which style is the best to make sure we enforce it in the future. This can be done by documenting the current implementations. |
I open a RFC #1503 to discuss about custom request migration. FMPOV, I think that we can probably change the behaviour of existing current request to deal with |
Currently implIntf and inferIntf accept a DocumentUri while typedHoles accepts
When looking at how vscode-ocaml-platform handles it:
I don't see any reason to not have the three requests have the same type of parameter