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

Make things private #69

Merged
merged 5 commits into from
Dec 14, 2015
Merged

Make things private #69

merged 5 commits into from
Dec 14, 2015

Conversation

dentarg
Copy link
Collaborator

@dentarg dentarg commented Dec 11, 2015

  • #new and .internal_parse
  • Constants

Close #60.

@dentarg
Copy link
Collaborator Author

dentarg commented Dec 11, 2015

rake profile:normalize reports Total: 1.791179 for master and Total: 2.063033 for this branch

@dentarg
Copy link
Collaborator Author

dentarg commented Dec 11, 2015

screen shot 2015-12-11 at 20 11 56

@walro
Copy link
Contributor

walro commented Dec 11, 2015

rake profile:normalize reports Total: 1.791179 for master and Total: 2.063033 for this branch

I am guessing due to the extra three calls in .parse compared to .internal_parse. I think it's fine though.

LGTM!

@walro
Copy link
Contributor

walro commented Dec 11, 2015

I am guessing due to the extra three calls in .parse compared to .internal_parse. I think it's fine though.

Ah, no, I think we make an extra parsing now, I think we should fix that.

@@ -101,7 +99,7 @@ def normalized
normalized_url.host = normalized_host
normalized_url.path = normalized_path

self.class.internal_parse(normalized_url)
self.class.parse(normalized_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will convert an addressable uri to a string (in .parse) and then back to a addressable uri (in .internal_parse). I think we want to avoid that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's pretty easily amended by splitting internal_parse into two different ones, one for addressable and one for public suffix

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's pretty easily amended by splitting internal_parse into two different ones, one for addressable and one for public suffix

We already had a helper method apparently, I thought the change in 219cf37 made sense

@walro
Copy link
Contributor

walro commented Dec 11, 2015

Would be interesting to re-run the benchmarks now

@dentarg
Copy link
Collaborator Author

dentarg commented Dec 11, 2015

Would be interesting to re-run the benchmarks now

Total: 1.793219

@walro
Copy link
Contributor

walro commented Dec 11, 2015

Cool

@dentarg
Copy link
Collaborator Author

dentarg commented Dec 14, 2015

@walro are we done? :)

@walro
Copy link
Contributor

walro commented Dec 14, 2015

I think so!

dentarg added a commit that referenced this pull request Dec 14, 2015
@dentarg dentarg merged commit 47de885 into master Dec 14, 2015
@dentarg dentarg deleted the privatize branch December 14, 2015 09:27
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

2 participants