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

Addressable spring clean #199

Merged
merged 3 commits into from
Dec 1, 2014

Conversation

iainbeeston
Copy link
Contributor

This tidies up a couple of (very?) minor things that came in with #174, that have caught my eye recently.

@RST-J I'd love to get your feedback on this, especially

@RST-J
Copy link
Contributor

RST-J commented Nov 30, 2014

In general this should be fine. When I introduced addressable I did not look for such things especially and looking at it, I don't see a reason to parse the URI twice.
But, you should have a look at whether join or merge is the right action here (+ is an alias of join in addressable).
I think I used merge when introducing addressable where before join was used with URI. And if I remember correctly I came up with this because otherwise some tests failed.

@iainbeeston
Copy link
Contributor Author

Thanks, that's a good tip - I'll check the addressable docs and look at how we're using it across the codebase=

@pd
Copy link
Contributor

pd commented Nov 30, 2014

👍

@iainbeeston
Copy link
Contributor Author

@RST-J I've had a look and I agree - it looks like merge(path: ...) would be better than join here (and also in Validator#absolutize_ref_uri) but I also found that tests fail when I tried doing that. I assume we're adding fragment or query elements to the path too.

So for now, I've changed + to join (it's more explicit and more in-keeping with everywhere else that we use Addressable::URI).

Otherwise I think I'm finished with this PR.

@RST-J
Copy link
Contributor

RST-J commented Dec 1, 2014

Ok, then this should be handled in an issue/PR on its own.
👍

RST-J added a commit that referenced this pull request Dec 1, 2014
@RST-J RST-J merged commit 7c99080 into voxpupuli:master Dec 1, 2014
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.

None yet

3 participants