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

Add Twingly::URL#ttld, "true TLD" getter #88

Merged
merged 5 commits into from
Aug 31, 2016
Merged

Add Twingly::URL#ttld, "true TLD" getter #88

merged 5 commits into from
Aug 31, 2016

Conversation

dentarg
Copy link
Collaborator

@dentarg dentarg commented Aug 30, 2016

Many1 ccTLDs have introduced a second level underneath their ccTLD, but
often, we don't care about the second level. We want the "true TLD".

Discussed in https://github.com/twingly/wally/issues/49.

Many[1] ccTLDs have introduced a second level underneath their ccTLD, but
often, we don't care about the second level. We want the "true TLD".

Discussed in https://github.com/twingly/wally/issues/49.

[1]: https://en.wikipedia.org/wiki/Second-level_domain
@@ -35,6 +35,11 @@
it { is_expected.to eq("") }
end

describe "#ttld" do
subject { url.tld }
Copy link
Contributor

Choose a reason for hiding this comment

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

tld -> ttld

@dentarg
Copy link
Collaborator Author

dentarg commented Aug 30, 2016

Could #true_tld be a better name?

@jage
Copy link
Contributor

jage commented Aug 31, 2016

Could #true_tld be a better name?

Don't think it helps, still an ambiguous name that you need to "learn". Might be easier to differentiate from tld, but you still need to read carefully since sld, tld, trd are also similar.

@dentarg
Copy link
Collaborator Author

dentarg commented Aug 31, 2016

Sure, we can keep it as #ttld.

@@ -100,6 +100,10 @@ def tld
public_suffix_domain.tld
end

def ttld
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment why we call it ttld?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would the comment say? Wouldn't it be better to call the method true_tld then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jage Like this?

    # Many ccTLDs have a second level[1] underneath their ccTLD, use this when
    # you don't care about the second level.
    # 
    # [1]: https://en.wikipedia.org/wiki/Second-level_domain
    def ttld
      tld.split(".").last
    end

Copy link
Contributor

Choose a reason for hiding this comment

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

What would the comment say? Wouldn't it be better to call the method true_tld then?

Nah, I don't think true is any better. "True" doesn't add any context when to use this.

@jage Like this?

Yes, that's better.

@jage
Copy link
Contributor

jage commented Aug 31, 2016

Just #88 (comment), otherwise this LGTM!

@@ -171,6 +171,11 @@ def valid_urls
it { is_expected.to eq("co.uk") }
end

describe "#ttld" do
subject { url.ttld }
it { is_expected.to eq("uk") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe me being a 🐔 but I think I'd like a test for #ttld when we have a normal domain as well, like a simple .com. I am thinking to expose problems in the way we split the #tld.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@walro
Copy link
Contributor

walro commented Aug 31, 2016

LGTM!

@dentarg dentarg merged commit 7e7ffcc into master Aug 31, 2016
@dentarg dentarg deleted the true-tld branch August 31, 2016 14:56
dentarg added a commit that referenced this pull request Aug 31, 2016
To release #88
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