Closed dkjhanitt closed 8 years ago
87.28%
Merging #109 into master will decrease coverage by -5.29% as of
14cc1a0
@@ master #109 diff @@
======================================
Files 21 21
Stmts 229 236 +7
Branches 13 14 +1
Methods 0 0
======================================
- Hit 212 206 -6
Partial 0 0
- Missed 17 30 +13
Review entire Coverage Diff as of
14cc1a0
Powered by Codecov. Updated on successful CI builds.
Hi @dkjhanitt,
Thanks for the PR.
I am reluctant to add guava as a dependency just for this feature. Most people using scala-uri will be not be using our publicSuffix
functionality and I feel it would be unfair to add such a large library to their classpaths. As you demoed in #108 it's straightforward to use guava alongside scala-uri, so I think it is best left as such. Let me know what you think!
As a side note: I also noticed that the guava implementation does not support newer gTLDs:
InternetDomainName.from("get.love").publicSuffix()
res1: com.google.common.net.InternetDomainName = null
Where as scala-uri version 0.4.12-SNAPSHOT
does, along with everything on the Public Suffix List. Let me know if this is of benefit to you?
Uri.parse("http://get.love").publicSuffix
res1: Option[String] = Some(love)
Hi @theon , I was more inclined to use Guava because in this case I do not need to manage domain list. In addition to this it comes with other good features that I may have to use... What you implemented is solving the purpose as well. Thanks, Deepak
Ok, great. I think if you are already using guava in your project it's a good shout. I will do some more work on the scala-uri implementation and make a decision whether to include it in a release, or just recommend people use guava. I'd like to know how you get on, with whichever implementation you use.
Summary of the changes