On 24.3.12 11:17, Jukka Zitting wrote:
Hi,
On Fri, Mar 23, 2012 at 7:48 PM, Michael Dürig<[email protected]> wrote:
- I think we should add a method to PropertyState for accessing the value as
Scalar. However this would introduce a circular dependency between oak-core
and oak-mk. How should we proceed here?
For now I was using the tree model interfaces from oak-mk to avoid
having to maintain identical copies in oak-core.
If we have a good case (e.g. handling of values) of why the internal
tree model should look different in -core than in -mk, then we should
come up with separate tree interfaces in -core.
On the other hand, if we restrict Scalars to just a strongly typed
version of JSON values then it might as well live inside .mk.model
where it could come in handy if we want to do stuff like treat
"\u0058" and "X" as equal when merging concurrent changes, etc.
That's actually the way I look at it. However moving Scalar to oak-mk
would require us to have a dependency from oak-jcr to oak-mk which I
think we shouldn't.
- Does it really make sense to allow for multiple Scalar implementations?
Shouldn't we just have one final class for that?
The ScalarImpl you wrote already consists of eight classes. The one
Thomas has in .oak.query is a single class but has more memory
overhead. There might also be a case for lazy scalars that only parse
the underlying JSON representation when actually needed. That's a
whole lot of implementation detail that I really would like to keep
clearly separate from the API.
To me Scalar is just a data object so having possibly different
implementations just adds unnecessary complexity. (Like always, I'm
afraid of broken equals ;-)) While I see the need for lazy loading, we
could just as well provide factory methods which take a thunk to the
value instead of the value itself. Like for binaries in ScalarImpl:
public static Scalar binaryScalar(final Callable<InputStream>
valueProvider){}
But let's not get caught up with this. I can live with Scalar being an
interface.
PS. .oak.ScalarImpl.java is missing the Apache license header.
Sorry about that, fixed it. IntelliJ's Copyright profiles don't seem to
work any more and I have kind of given up on this.
Michael
BR,
Jukka Zitting
