-
Notifications
You must be signed in to change notification settings - Fork 4
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
Exact DSL draft #6
Conversation
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.
Thank you for taking the time to initiate the work @ustits! Overall, great design - I like it 👍
Added a couple of suggestions around:
- reducing boilerplate
- Arrow.
Raise
- naming improvements
Please, let me know what you think. The goal is discussion and perfection through iterations and feedback.
} | ||
} | ||
|
||
fun <A, B> exact(predicate: Predicate<A>, constructor: (A) -> B): Exact<A, B> { |
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.
Two things:
- I'd suggest switching the argument order like
exact(constructor, predicate)
so we can do:
companion object : Exact<Int, PositiveInt> by exact(::PositiveInt) { it > 0 }
- Do we want
exact
to work only with predicates? I'm thinking about overloading a version `fun exact(constr: (A) -> B, constraint: (A) -> Either<ExactError, B>)
The benefit of this is that we can define types like NotBlankTrimmedString
with much less boilerplate.
companion object : Exact<String, NotBlankTrimmedString> by exact(::NotBlankTrimmedString) {
it.trim().takeIf(String::isNotBlank) ?: raise(...)
}
On this, @ustits @nomisRev should we better use the new Raise
API instead of Either for the method of Exact
that must be implemented.
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 think having it Raise
based makes sense. In that case I would go for:
fun exact(constr: (A) -> B, constraint: Raise<ExactError>.(A) -> B)
companion object : Exact<String, NotBlankTrimmedString> by exact(::NotBlankTrimmedString) {
ensureNotNull(it.trim().takeIf(String::isNotBlank)) { ... }
}
but both should exclude each-other 🤔 They shouldn't conflict with each other if we use OverloadResolutionByLambdaReturnType
.
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.
@ILIYANGERMANOV Unfortunately we can't implement it in such a way, kotlin compiler thinks that after {
there must be a class body, not a lambda, so eventually there is no difference in arguments order. On the other hand exact
can be used not only in delegation but also as regular method call:
fun main() {
exact(::NotBlankString) {
it.isNotBlank()
}
}
But it is rather synthetic example, doesn't look like a real use case
@nomisRev I am not quite familiar with Raise api, can you tell me where to look for implementation examples?
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.
Thank you for the explanation @ustits! I did all the code examples from my Fold w/o an actual IDE/compiler - apologies for that! Regarding Raise
- I'd recommend:
- https://arrow-kt.io/learn/typed-errors/working-with-typed-errors/
- KDoc for
Raise#raise
,Raise#ensure
andRaise#ensureNotNull
arrow#3038 (I've put some KDoc for Raise with short examples if you're looking for a TL;DR;)
Raise TL;DR;
- like
throws
but type-safe, in order to raise you need aRaise<A>
context - to "catch" raise use
recover
- interoperable with
Either
- @nomisRev can give you a better explanation and details but that should be enough to get you started
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 @ILIYANGERMANOV , can you tell me, am I moving in the right direction? As I understood I need to use recover
on the constraint and map the result onto Either
. Btw in such scenario probably we don't need a constructor as an argument:
fun <A, B> exact(constraint: Raise<ExactError>.(A) -> B): Exact<A, B> {
return object : Exact<A, B> {
override fun from(value: A): Either<ExactError, B> {
return recover({
constraint(value).right()
}) {
error -> error.left()
}
}
}
}
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.
@ustits yes, it looks good to me! Thank you for making the change :)
A valid example usage would look like this, right?
companion object : Exact<String, NotBlankTrimmedString> by exact({
ensureNotNull(it.trim().takeIf(String::isNotBlank)?.let(::NotBlankTrimmedString)) {
ExactError("Cannot be blank!")
}
})
companion object : Exact<Float, Percent> by exact({
Percent(it.coerceIn(0f, 1f))
})
I like this API a lot! It's a lot simpler when we get rid of the constructor.
The constructor can be required only for the predicate exact
case.
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.
@ustits instead of using recover
with left
and right
you can also just use the either
block. It works through Raise
😉. TL;DR Raise
is a higher-level abstraction that can work over errors of E
. So for Either
it's just E
, but for Option
it's for example None
or typealias Null = Nothing?
for _nullable types`.
return either { constraint(value) }
💡 In a nutshell, we have two types of operations:
TL;DR;
This is an overkill but just sharing for brainstorming purposes. companion object : Exact<String, CustomerId> by exact()
.mustBe(String::isNotBlank) // predicate
.transform(String::trim) // transformation
.transformOrFail { // both
when {
it.startsWith("u/") -> User(it.drop(2))
it.endsWith("@arrow"l -> Admin(it)
else -> fail("Invalid id: $it")
}
}
.mustBe { ... }
.transform { it.uppercased() }
.build(::CustomerId) |
For visibility mentioning #4 |
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.
Really nice work @ustits 🙌 Awesome to get a community effort going here 😍
I left some nits mostly, and some thoughts but I like everything I saw here.
fun fromOrThrow(value: A): B { | ||
return when (val result = fromOrEither(value)) { |
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.
nit: I use expression bodies wherever possible 😁 I feel it makes it more explicit.
} | ||
} | ||
|
||
fun <A, B> exact(predicate: Predicate<A>, constructor: (A) -> B): Exact<A, B> { |
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 think having it Raise
based makes sense. In that case I would go for:
fun exact(constr: (A) -> B, constraint: Raise<ExactError>.(A) -> B)
companion object : Exact<String, NotBlankTrimmedString> by exact(::NotBlankTrimmedString) {
ensureNotNull(it.trim().takeIf(String::isNotBlank)) { ... }
}
but both should exclude each-other 🤔 They shouldn't conflict with each other if we use OverloadResolutionByLambdaReturnType
.
@ustits I invited you to the repo, so you can also create branches directly not his repo. |
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.
The design is already good! I think we can merge it and build on top of it. @ustits feel free to merge 👍 Great work! 👏
After merging this one, we can start creating small PRs with minor improvements and have discussions inside them.
Agreed, let's merge this PR 💪 @ustits or @ILIYANGERMANOV can you confirm that you can merge this PR with the proper approvals? Just to know I gave you the correct permissions to the repo 😅 |
@nomisRev @ustits I merged it - we have the correct permissions. Wanted to merge it directly because I want to play around and build on top of it - it's more convenient to have it merged instead of doing stacked PRs. |
@ILIYANGERMANOV @nomisRev Thanks for the help! I haven't included changes with Raise, will do a separate PR for this |
Thank you so much @ustits & @ILIYANGERMANOV! 🙌 Super exciting to see this taking shape 😍 🥳 |
Usage example can be found in
NotBlankTrimmedString
in test package.Exact
. It both keeps a constraint and helper functions for creating "exact" typefrom
instead ofcreate
because, imho, it sounds more natural, e.g.NotBlankString.fromOrThrow(str)
versusNotBlankString.createOrThrow(str)
. Don't have a strong opinion here so it can be changedExact
an interface otherwise the clients won't be able to extend from other classes if neededExact
: interface implementation andexact()
method callExact
can be combined viaand
function likeNotBlankString and TrimmedString