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

Stop using deprecated URL constructors #1529

Closed
wants to merge 1 commit into from

Conversation

akurtakov
Copy link
Member

Changes tests only.

Copy link

Test Results

  281 files  + 79    281 suites  +79   47m 17s ⏱️ + 8m 35s
3 586 tests ±  0  3 498 ✅  -  12   76 💤 ± 0  0 ❌ ±0  12 🔥 +12 
8 280 runs  +932  8 061 ✅ +866  207 💤 +54  0 ❌ ±0  12 🔥 +12 

For more details on these errors, see this check.

Results for commit 1d50889. ± Comparison against base commit 0d51f20.

@merks
Copy link
Contributor

merks commented Dec 19, 2024

Hopefully the tests don't have URIs with spaces. What if they run in a workspace where the installation or workspace (e.g., user account) has spaces?

@merks
Copy link
Contributor

merks commented Dec 19, 2024

@HannesWell

I wonder is there some URIUtil somewhere that can be used safely with strings that are safe for new URL but not safe for new URI or URI.create?

I've not actually looked if the concern is relevant here specifically. Too much to do...

@akurtakov
Copy link
Member Author

@merks Would you please share some links with details if you have handy? I'm surprised that the new recommended way is more fragile than the deprecated one.

@merks
Copy link
Contributor

merks commented Dec 19, 2024

I've seen folks getting bitten by this on several separate occasions. Only the second one throws an exception.

		String string = "file:/Users/Joe Shmoe/";
		new URL(string);
		URI.create(string).toURL();

Perhaps/likely there are other characters, but the space is the most glaring and most likely to occur.

It's a bit of a disservice to deprecate something without a description of the gotchas involved with the proposed alternative. ☹️

@HannesWell
Copy link
Member

HannesWell commented Dec 19, 2024

I wonder is there some URIUtil somewhere that can be used safely with strings that are safe for new URL but not safe for new URI or URI.create?

I started an effort for a robust URL-> Path conversion in eclipse-equinox/equinox#691, which probably could be extended to convert from String too (if it's not yet present). I just didn't have the time yet to complete it.

In this case I think this would better be handled by using the actual type of the value: Path; as proposed in:

With the latter this should be obsolete.

@akurtakov
Copy link
Member Author

I'm closing this one as there seem to be multiple things that make this one just uninformed noise.

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.

3 participants