Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattiasdrp
Copy link

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) ()
    
  • For typedHoles
    let uri = TextDocument.uri doc
    

I don't see any reason to not have the three requests have the same type of parameter

@mattiasdrp
Copy link
Author

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
@mattiasdrp mattiasdrp force-pushed the mattias@refactor_implintf_and_inferintf branch from d6de128 to 7bb7fb9 Compare March 12, 2025 13:36
@voodoos
Copy link
Collaborator

voodoos commented Mar 12, 2025

I don't see any reason to not have the three requests have the same type of parameter

That's quite right, but is that a reason sufficient-enough to introduce a breaking change in the implIntf and inferIntf custom request ? I personally don't think that it is worth-it.

@mattiasdrp
Copy link
Author

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 :-)

@nemethf
Copy link

nemethf commented Mar 12, 2025

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.

@mattiasdrp
Copy link
Author

I like this idea @nemethf, thanks for it! I'll improve my PR tomorrow!

@voodoos
Copy link
Collaborator

voodoos commented Mar 13, 2025

Having to already play with inconsistencies when there are only 3 different requests

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.

@xvw
Copy link
Collaborator

xvw commented Mar 13, 2025

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 uri | {uri: DocumentUri} in order to be more lax and to not have to compose with client migration ATM.

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.

4 participants