CodeLionX / actordb

Actor Database System Framework using Akka
MIT License
0 stars 2 forks source link

Get rid of Option, when accessing record columns #99

Closed CodeLionX closed 6 years ago

CodeLionX commented 6 years ago

Proposed Changes

Is this reasonable??

Map signatures:

For a scala.immutable.Map[K,+V] the signatures look like the following (defined in MapLike[K, V, This]):

srfc commented 6 years ago

This is an anti-pattern in scala, I think? If I understand you right the MapLike#apply has the same interface as the one you are suggesting for Record#get, right?

What I could imagine is also using Record#apply as an accessor - Record is a collection semantically anyhow - and make it unsafe (i.e. throwing a NoSuchElementException or returning the value) and have it alongside Record#get.

CodeLionX commented 6 years ago

Ok, I understand your point. Do you think it's beneficial to have both ways to access the cells in a Record?

Maybe we can inherit from MapLike using ColumnDef[T] instead of UntypedColumnDef. I'll have a look.

srfc commented 6 years ago

I did some more quick research (https://stackoverflow.com/a/26215516/2161364) because the idea of having to unpack multiple Options from e.g. messages bothered me but came up with what is probably the more scala-ic way of doing something like this:

val response = for {
  v1 <- someRecord.get[Type1](Column1Def)
  v2 <- someRecord.get[Type2](Column2Def)
  v3 <- maybeSomeOtherRecord.get[Type3](Column3Def)
} yield // <some response depending on the values from my two Records>

response getOrElse // handle your error somehow

Or you can obviously stuff them in a sequence and call forEach if you don't want to fail as soon as one of the values is None.

So maybe we should scrap the non-Option wrapped returns altogether?

I can't really find any answer on why MapLike[K,V] provides the unsafe apply accessor.

CodeLionX commented 6 years ago

Please see #101. This changes the required methods to:

This is now a completely independent API, but heavily based on the original scala MapLike one. We now still have both accessors, but no overloading anymore. We can easily get rid of one of the accessor if we like to.

This PR can be closed as early as the discussion is resolved.

srfc commented 6 years ago

It's resolved here for me. We can keep the apply[T]() accessor for now but to see if it is useful in certain scenarios but think about alternatives primarily.