Skip to content

fix: remove decoder service #2415

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

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

ap891843
Copy link
Contributor

@ap891843 ap891843 commented Aug 1, 2024

This service was originally introduced due to compatibility issues in the format of URI's send by the client (was never a good idea). After several refactoring at the client side, we adopted to UNC file format and this service should no longer be required. In fact this decode service would lead to the in-consistency in the way we represent URI's on the LSP server. Its time for it to go, RIP

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test no regressions introduced

Checklist:

  • Each of my commits contains one meaningful change
  • I have performed rebase of my branch on top of the development
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@ap891843 ap891843 marked this pull request as ready for review August 2, 2024 09:12
@slavek-kucera
Copy link
Contributor

With this change the server relies on the lsp client to send normalized URIs. Will that be a problem? Should the incoming URIs be normalized e.g. in the LSP message handlers?

@ap891843
Copy link
Contributor Author

ap891843 commented Aug 2, 2024

With this change the server relies on the lsp client to send normalized URIs. Will that be a problem? Should the incoming URIs be normalized e.g. in the LSP message handlers?

Yes. This was the initial idea.
First step was to do some clean up - get rid of UriDecodeService. It was poorly implemented and was breaking a lot of cases

Then decide the appropriate place to normalize the URI's. I am still not sure, what would be the correct place for it. Should it sit on client or the LS server ?
For example:

  • custom URI object on LS <-- this would help us overcome some of the java limitation, but could be extensive
  • LS middle ware client <-- Doubtful as it break one server , multi client
  • LSP message handlers , as you suggested

@ishche
Copy link
Contributor

ishche commented Aug 2, 2024

  • LSP message handlers , as you suggested

By LSP values we should minimize client specific changes. I like idea with custom URI class, for now it can be wrapper on string with cleanup constructor.

@ap891843
Copy link
Contributor Author

ap891843 commented Aug 2, 2024

With this change the server relies on the lsp client to send normalized URIs. Will that be a problem? Should the incoming URIs be normalized e.g. in the LSP message handlers?

fyi, tested it on shared drive ( windows shared drive using hostname in URI and basic testing on mac and win). Seems to work. But fails for path like //./c:, which is a known issue

@slavek-kucera
Copy link
Contributor

I'd also prefer a special type (value class) that would contain the normalized uri - I expect a lot of changes in the rest of the server though.

Nurkambay
Nurkambay previously approved these changes Aug 8, 2024
@Nurkambay Nurkambay self-requested a review August 8, 2024 11:42
@Nurkambay
Copy link
Contributor

Let's add unit tests first if we can break something with this PR. For example, if we suppose that the Client will send us a normalized URI, let's add unit tests that are failing if received URI is not normalized

@Nurkambay Nurkambay dismissed their stale review August 8, 2024 11:45

Unit tests are needed

@ap891843 ap891843 force-pushed the remove_decoder branch 3 times, most recently from 5df4d8a to 6a9db86 Compare August 8, 2024 14:20
Signed-off-by: ap891843 <[email protected]>
@ap891843
Copy link
Contributor Author

ap891843 commented Aug 9, 2024

Let's add unit tests first if we can break something with this PR. For example, if we suppose that the Client will send us a normalized URI, let's add unit tests that are failing if received URI is not normalized

updated

@ap891843 ap891843 merged commit b85d3cd into eclipse-che4z:development Aug 15, 2024
17 checks passed
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