-
Notifications
You must be signed in to change notification settings - Fork 41
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
Implement constraints using Comparable
interface
#181
Implement constraints using Comparable
interface
#181
Conversation
Here's a first draft @dhoepelman; I have included your feedback on the issue.
|
36127a2
to
f919d00
Compare
Alright. I just brought back the original implementations of the now deprecated |
f919d00
to
8d30d46
Compare
@@ -13,26 +14,34 @@ public fun <T : Number> ValidationBuilder<T>.multipleOf(factor: Number): Constra | |||
} | |||
} | |||
|
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.
👍
Can you add overloads for Long
too? Float seems like a more niche use-case so feel free to remove, keeping them is fine now you made them.
// The below functions are overloads of the Comparable<T> functions, provided for backwards | |
// compatibility and convenience for the common case where a number is constrained by an Int |
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.
Can you add overloads for
Long
too? Float seems like a more niche use-case so feel free to remove, keeping them is fine now you made them.
Kotlin automatically converts the literal to a Long
value depending on the context:
konform/src/commonTest/kotlin/io/konform/validation/constraints/ConstraintsTest.kt
Lines 154 to 157 in 8d30d46
assertEquals( | |
Valid<Long>(10), | |
Validation<Long> { maximum(10) }(10) | |
) |
That's why I was thinking an explicit overload might not be needed. I haven't tried with a value explicitly typed as Int
though. I will check that and can certainly add them.
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.
ah I see, looked at the widening rules but integer literal to long does happen automatically, so shouldn't be needed
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.
Ah, sorry. Saw your message a moment too late and just pushed an update adding the overload for Long
. 🙈
I checked the case where the limit value is explicitly typed, e.g., when is provided using a constant value from elsewhere:
val MAX: Int = 100
val validation = Validation<Long> {
maximum(MAX)
}
In this case the overload still might be useful.
Thanks! Looks good at a quick glance. Will find some time soon to do full review and merge. |
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.
Thanks!
Allow building the constraints - `maximum` and `exclusiveMaximum`, - `minimum` and `exclusiveMinimum` for any type `T`, using a `Comparable<T>`. Previously, these constraints were only available for subtypes of `Number` and relied on converting to `Double` for comparing numbers of different types. This could cause issues when comparing values which cannot be cleanly converted from the original number type. For compatibility and convenience, we provide overloads for these constraints that accept an `Int` as the limit value as this seems like a rather common case. Closes konform-kt#180
Head branch was pushed to by a user without write access
abec3a0
to
24eb7d6
Compare
@pmeinhardt Can you either restore or incorporate in your own commits the fixes from |
You mean cherry-pick them here? Sure thing. 🏃 |
Authored-by: David Hoepelman <[email protected]>
Authored-by: David Hoepelman <[email protected]>
Authored-by: David Hoepelman <[email protected]>
D'uh. Had to apply them as patches, as Git would not fetch those commits. Sorry about that and thanks a ton for the fixes. I haven't taken the time yet to set up all platforms and ran only the JVM tests locally. 🫶 |
Allow building the constraints
maximum
andexclusiveMaximum
,minimum
andexclusiveMinimum
for any type
T
, using aComparable<T>
.Previously, these constraints were only available for subtypes of
Number
and relied on converting toDouble
for comparing numbers of different types. This could cause issues when comparing values which cannot be cleanly converted from the original number type.For compatibility and convenience, we provide overloads for these constraints that accept an
Int
as the limit value as this seems like a rather common case.Closes #180