-
Notifications
You must be signed in to change notification settings - Fork 157
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 IPv4 and private networks #356
Conversation
89bc8d9
to
987b879
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some minor comments but in general this LGTM. You should also add MiMa exclusions for the things it complained about. Adding methods is OK.
@@ -40,14 +43,34 @@ object string extends StringValidate with StringInference { | |||
final case class XPath() | |||
} | |||
|
|||
private[refined] trait StringValidate { | |||
object Predicates { | |||
object IPv4 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move the IPv4
object into the string
object right below the IPv4
case class and remove the Predicates
object? It makes sense to put auxiliary code of predicates into their proper companion.
|
||
/** A `String` representing a valid IPv4 in a private network according to RFC1918 */ | ||
type Rfc1918Private = | ||
String Refined Rfc1918ClassAPrivateSpec Or Rfc1918ClassBPrivateSpec Or Rfc1918ClassCPrivateSpec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add an alias type Rfc1918PrivateSpec = Rfc1918ClassAPrivateSpec Or Rfc1918ClassBPrivateSpec Or Rfc1918ClassCPrivateSpec
? That could be used here and in the definition of PrivateNetwork
. A type Rfc5737TestnetSpec
could be also useful to avoid repetition.
987b879
to
f8e23af
Compare
Codecov Report
@@ Coverage Diff @@
## master #356 +/- ##
==========================================
+ Coverage 98.71% 98.74% +0.02%
==========================================
Files 39 39
Lines 468 478 +10
Branches 8 8
==========================================
+ Hits 462 472 +10
Misses 6 6
Continue to review full report at Codecov.
|
Looks great, many thanks @NeQuissimus! |
Basically a follow-up to #353
This is what I am using at work, I figured I would clean it up and contribute it back.
No external Java dependencies, so this also works with Scala.js without any issues.
I will have to add IPv6 next week, so I will send that in as well once I have it.
/cc @derekmorr @fthomas