Skip to content
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

Implement CanonicalUriProvider API. Fixes #12735 #12743

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

tsmaeder
Copy link
Contributor

What it does

The implementation follows the usual pattern of having a Theia service for canonicla URI's with the VS Code implementation being a special case of the Theia service contributions.
The API is proposed in VS Code, but used in built-in extensions in versions >= 1.80.0

Contributed on behalf of STMicroelectronics

How to test

Add the attached extension to the plugins folder. It provides two commands: one to "Register Canoniccal URI Provider" and one to "Convert to Canonical URI". The first one registers a provider we can test with the second command. To test, invoke the command, then

  1. Select a target scheme ('uppercase/lowercase/https')
  2. Enter a uri
  3. The provider converts the uri either to upper/lower case depending on the chosen target scheme. It returns undefined if the targets scheme is https.

Review checklist

Reminder for reviewers

The implementation follows the usual pattern of having a Theia service
for canonicla URI's with the VS Code implementation being a special case
of the Theia service contributions.
The API is proposed in VS Code, but used in built-in extensions in
versions >= 1.80.0

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder
Copy link
Contributor Author

here's the vsix:

canonicaluritest-0.0.1.zip

and here's the source:

canonicaluritest.zip

@tsmaeder
Copy link
Contributor Author

alternatively, you could also test with the 1.80.0 version of the "github" built-in. Here's a prebuilt copy:

github-1.80.0.zip

for this to works, you'll need to replace the git package in the theia example app with the git and git-base built-ins: remove the dependency on git and remove the exclusions of the built-ins from the main package.json. Don't forget to "donwload:plugin" and to replace the 1.76.0 version of the github built-int with the version attached here.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always getting The canonical URI is undefined for every URI that I enter for every option (uppercase/lowercase/https).

vscode.Uri.parse(decodeURI(uri.toString()))

I believe the code above (extracted from the extension source) isn't correct. Uri.parse expects a string that has been created by calling uri.toString(). If you run decodeURI in between that, you create an invalid URI string that can no longer be parsed.

@tsmaeder
Copy link
Contributor Author

I'm always getting The canonical URI is undefined for every URI that I enter for every option (uppercase/lowercase/https).

vscode.Uri.parse(decodeURI(uri.toString()))

I believe the code above (extracted from the extension source) isn't correct. Uri.parse expects a string that has been created by calling uri.toString(). If you run decodeURI in between that, you create an invalid URI string that can no longer be parsed.

This works for me. Did you call the "Register Canonical URI Provider" command before testing the conversion?

@tsmaeder tsmaeder mentioned this pull request Jul 24, 2023
1 task
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the source code, it seems like only URIs with the testScheme scheme are viable as input for the extension. The test instructions only mentioned:

Enter a uri

Which didn't work for any old URI I tried. Using testScheme works as expected though.

Looks good to me 👍

@tsmaeder
Copy link
Contributor Author

Looking at the source code, it seems like only URIs with the testScheme scheme are viable as input for the extension. The test instructions only mentioned:

Enter a uri

That's my 🤦

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder tsmaeder merged commit a8c4954 into eclipse-theia:master Jul 24, 2023
@vince-fugnitto vince-fugnitto added this to the 1.40.0 milestone Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants