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
Issue-1177 TArray #1191
Issue-1177 TArray #1191
Conversation
@jdegoes I've opened this for quick review. Could you just tell me if I'm generally doing it right or is this totally missed shot? |
@LGLO Yes, generally speaking, this is right on! Nice work. 👍 |
c394a7f
to
c56f472
Compare
Should I re-write tests to new framework? |
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.
Looking great! Just a few minor tweaks / questions and will be good to ship.
this.foldM(())((_, a) => f(a)) | ||
|
||
/** Creates [[TArray]] of new [[TRef]]s, mapped with pure function. */ | ||
def map[B](f: A => B): STM[Nothing, TArray[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 do not think I would include map
or mapM
. If necessary we could include a duplicate
method to create a copy of the array, then the user could use transform
/ transformM
on the duplicate.
How does that sound?
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 can't see it working, because duplicate
and transform
wouldn't/don't change type of TArray[A]
.
transform
(named after Array#transform
) updates values inside Ref
s.
Maybe I should rename transform
to updateAll
?
STM.foreach(array)(_.get.flatMap(f).flatMap(b => TRef.make(b))).map(l => new TArray(l.toArray)) | ||
|
||
/** Atomically updates all [[TRef]]s inside this array using pure function. */ | ||
def transform(f: A => A): STM[Nothing, Unit] = |
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'd make all methods final
.
} | ||
|
||
/** Updates element in the array with given function. None signals index out of bounds. */ | ||
def update(index: Int, fn: A => A): STM[Nothing, Option[A]] = |
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.
We could also use STM.die
for out of bounds. Thoughts?
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.
Yeah, I think it will be better. User is still obliged to take care of it but not necessarily just after calling this method.
👍
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.
Umm, why die
and not fail
?
@jdegoes please take a look at 97a7d65 and tell me which parts do you like and which you don't like. I see a lot of changes in tests. Should I migrate |
@LGLO I would not port tests yet. We can do that later after ZIO Test is further along. |
@jdegoes OK - I'll leave tests refactoring for some later time. |
@LGLO I would prefer to |
@jdegoes I've switched to |
@LGLO Looks great! Sorry for the delay in getting this merged. |
Woops, some methods became deprecated so it doesn't compile on master. Will send a PR shortly. |
TArray[A]
wrapper forArray[TRef[A]]
List of functions (quick draft, for sure some will be added, maybe some will be removed):
TArray
A => Unit
)