-
Notifications
You must be signed in to change notification settings - Fork 474
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
Fix several questionable .relativeTo() results. #78
Conversation
I think you went a bit overboard with the merging thing. RFC 3986 celarly states in Section 5.2.2 - Transform References that there are 5 components to be considered: scheme, authority(username, password, hostname, port), path, query, fragment. As for normalizing both URIs prior to figuring out their relation: yep, good idea. if there's any other subtlety I've missed, please point me to it. Cheers, |
With one exception, The one exception is where Thanks for all your time today, btw. I’m sorry if I’m distracting you from more important work :). EDIT: I accidentally posted this too soon, so I’m about to post an additional comment with some more stuff I wanted to say. |
The cases that you didn’t mention are the ones I think are the most important: 'same scheme': URI("http://example.org/foo/bar/bat").
relativeTo("http://example.com/foo/").
toString();
'credentials' and 'base credentials' demonstrate a similar cases where the hostname is the same but the credentials are different. If any part of the authority differs between the URIs, then the only part that can be made relative is the scheme. 'different scheme':
'parent top level':
|
I will check tomorrow to make sure that my merging behaviour is the inverse of the relative URI resolution algorithm defined by RFC3986. One place where I have concerns is that I’m considering the credentials and hostname separately, whereas RFC3986 considers them together as the authority. Possibly my approach leads to some incorrect edge cases, although it should still be more correct than the existing behaviour, which ignores the authority altogether. |
Hi! I’m just dropping in to say that I will be having another look at this next week. I don’t think you should merge this PR as-is, as there are a couple of issues I need to look at more closely. |
Just leaving this here as a note to myself: Since the URI standard doesn’t define relativization, we have to define it for ourselves, preferably in a way that makes useful sense in the context of the existing URI standard. Since the URI standard does define relative URI resolution ( Therefore, my goal is that At the moment the code for this pull request doesn’t achieve this goal, although it is pretty close. Here are the known problems:
|
An additional though here: |
I think I might be missing something... I took it as read that any URI returned by (I’m not really sure how a URI could be invalid but still meet the conditions above – how do you absolutize a URI that isn’t representable?) ( |
There is one case where it might be worth returning a longer-than-necessary URI, though:
The shortest possible result here is the empty string, which is a valid URI, but I can imagine that it might cause trouble in some contexts. The next shortest canonically-equivalent URI is |
What's the status on this? are there still things missing? I've started merging the open stuff into master |
IMHO this is unambiguously better than the implementation it replaces:
There are still some things that could be improved, but they are problems with the existing implementation too:
tldr: I think you’re quite safe to merge this pull request. |
Oh wait, I need to check that I haven’t broken anything by attempting to merge the authority component. I’ll add tests for that now, and come back. |
…ble to the normalized form of the original URI.
OK, I added some more tests and they pass. I think this PR should be good to merge. |
I've merged this into master - it will be included in the next release. thank you for your help! |
I've fixed Issue #95 please go ahead and write up the relativization docs as proposed :) |
Thanks for fixing #75 – that fixed the specific cases that I raised.
However, there are several more things that I think aren’t quite right about
.relativeTo()
.May I humbly suggest the attached changeset?
Hopefully the test cases that I have added and changed speak for themselves, but I’m happy to explain my rationale if anything is controversial, or to be corrected if I’m being silly :).