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

patching #57, $uri->canonical unconditionally returns a clone #58

Merged
merged 1 commit into from
Jan 8, 2019

Conversation

doriantaylor
Copy link
Contributor

This change makes $uri->canonical uniformly return a cloned object. Erstwhile it returned $self if there was nothing to change, for the stated purpose of efficiency, although benchmarks suggest this turns out to be only about a 7% gain on an operation that is measured in microseconds, and only in the no-op scenario.

@karenetheridge
Copy link
Member

@doriantaylor can you squash these commits into one, and add "closes #57" to the commit message?

lib/URI.pm Outdated Show resolved Hide resolved
lib/URI.pm Outdated Show resolved Hide resolved
t/generic.t Outdated Show resolved Hide resolved
@doriantaylor
Copy link
Contributor Author

All requested changes made. Worth noting re $ref1 == $ref2 is all over the place internally, so if there's an overload on numeric == then those instances should probably be changed to refaddr too.

@oalders
Copy link
Member

oalders commented Dec 18, 2018

@karenetheridge there have been some changes. Can you review them?

@doriantaylor
Copy link
Contributor Author

are these changes satisfactory?

@oalders
Copy link
Member

oalders commented Jan 7, 2019

@doriantaylor not sure if @karenetheridge saw this. I'll touch base with her tomorrow so we can see about getting this merged.

@oalders
Copy link
Member

oalders commented Jan 8, 2019

Thanks @doriantaylor and @karenetheridge!

@oalders oalders merged commit c77b2bc into libwww-perl:master Jan 8, 2019
@oalders
Copy link
Member

oalders commented Jan 9, 2019

@doriantaylor I just reverted this change for the time being. Please see libwww-perl/HTTP-Message#121

Since this was breaking HTTP::Message installs I figured it would be best to back out of the changes for the time being until we know what the issue with HTTP::Config. Are you able to look into this?

@doriantaylor
Copy link
Contributor Author

Even money on the test being the problem. I'll check it out.

@doriantaylor
Copy link
Contributor Author

eh spoke too soon; looking at matching_items

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