incubator-clerezza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Henry Story <henry.st...@bblfish.net>
Subject Re: Reviewing svn commit: r1138602 (EasyGraph, Conversions)
Date Wed, 22 Jun 2011 21:22:54 GMT

On 22 Jun 2011, at 22:57, Reto Bachmann-Gmuer wrote:

> Hi Henry
> 
> Great to see the bar has tuned green again! Thanks for fixing this so quickly.

yes, I did not get to write up a summary for an IEEE conference keynote I need to do in August.
http://wi-iat-2011.org/

So I really need to get that done next.

More below.

> 
> See some comments to this patch inline. 
> 
> On Wed, Jun 22, 2011 at 9:59 PM, <bblfish@apache.org> wrote:
> Author: bblfish
> Date: Wed Jun 22 19:59:49 2011
> New Revision: 1138602
> 
> URL: http://svn.apache.org/viewvc?rev=1138602&view=rev
> Log:
> CLEREZZA-510 fix a merge issue
> A commit comment should be more informative, also it it is sometimes good to do multiple
commits for unrelated patches, here for example the reorganization of imports and the functional
changes

yes, these are just bumps on the road. I thought I had checked all in the first go, then I
discovered that in the meantime some changes had been made in the repo, which led to a merge
issue, which somehow seemed much smaller...

>  
> 
> Modified:
>    incubator/clerezza/trunk/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EasyGraph.scala
>    incubator/clerezza/trunk/parent/rdf.scala.utils/src/test/scala/org/apache/clerezza/rdf/scala/utils/EasyGraphTest.scala
> 
> Modified: incubator/clerezza/trunk/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EasyGraph.scala
> URL: http://svn.apache.org/viewvc/incubator/clerezza/trunk/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EasyGraph.scala?rev=1138602&r1=1138601&r2=1138602&view=diff
> ==============================================================================
> --- incubator/clerezza/trunk/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EasyGraph.scala
(original)
> +++ incubator/clerezza/trunk/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EasyGraph.scala
Wed Jun 22 19:59:49 2011
> @@ -21,16 +21,15 @@ package org.apache.clerezza.rdf.scala.ut
> 
>  import org.apache.clerezza.rdf.core._
>  import impl._
> -import org.apache.clerezza.rdf.ontologies.RDF
>  import java.math.BigInteger
>  import java.lang.Boolean
>  import java.net.{URL, URI}
>  import org.apache.clerezza.rdf.core._
> -import reflect.Apply
> -import org.apache.clerezza.rdf.utils.{UnionMGraph, GraphNode}
> -import java.util.Date
> +import org.apache.clerezza.rdf.utils.UnionMGraph
> +import org.apache.clerezza.rdf.utils.GraphNode 
> Here you're splitting the combined import stamement into two

I was having trouble with IntelliJ somehow putting red lines everywhere. In fact it still
is. I suppose I should not have upgraded to their beta releases...

> 
>  import scala.collection.mutable.HashMap
> -import scala.Option
> +import org.apache.clerezza.rdf.ontologies.{XSD, RDF} 
> +import java.util.{HashSet, Date}
> And here you're adding new combined import statements 

Same as above. Some IntelliJ oddness.

> 
>  object EasyGraph {
> 
> @@ -38,12 +37,8 @@ object EasyGraph {
> 
>        implicit def string2litBuilder(str: String) = new EzLiteral(str)
> 
> -       implicit def string2lit(str: String) = litFactory.createTypedLiteral(str)
> -
>        implicit def lit2String(lit: Literal) = lit.getLexicalForm
> 
> -       implicit def litBuilder2lit(litBuilder: EzLiteral) = litFactory.createTypedLiteral(litBuilder.lexicalForm)
> -
> See discussion below
> 
>  
>        implicit def date2lit(date: Date) = litFactory.createTypedLiteral(date)
> 
>        implicit def int2lit(int: Int) = litFactory.createTypedLiteral(int)
> @@ -75,13 +70,13 @@ object EasyGraph {
>  * An Easy Literal, contains functions for mapping literals to other literals, ie from
String literals to
>  * typed literals.
>  */
> -case class EzLiteral(lexicalForm: String) {
> +class EzLiteral(lexicalForm: String) extends TypedLiteral {
> 
> +       def unapply(lexical: String) = lexical
> I don't think instances of this class should be used in case-statements, so the unapply
method isn't needed imho, but you're right that it shouldn't be a case-class

Well it's so easy to add it here. It's a constant, and could be very useful. Don't see what's
wrong with it. 

>  
> 
>        /**
>         * @return a plain literal with language specified by lang
>         */
> -       //TODO get a better name for this
>        def lang(lang: Lang) = new PlainLiteralImpl(lexicalForm, lang)
>        def lang(lang: Symbol) = new PlainLiteralImpl(lexicalForm, new Language(lang.name))
//todo lookup in LangId instead
> Don't understand the TODO, but will once you've answered my other mail probably

The idea would be to have Lang have a hash of symbols so that they function as evolvable enums.
Mostly all the language or langauge-country tags should be available to the developer so he
does not need to look them up on the web and waste time finding RFCs, but if he wants to create
an artificial language he should be able to add one easily.
I was starting to work on that yesterday to see how that would look like.

>  
> 
> @@ -90,6 +85,18 @@ case class EzLiteral(lexicalForm: String
>        def uri = new UriRef(lexicalForm)
> 
>        def getLexicalForm = lexicalForm
> +
> +       override def equals(other: Any) = {
> +      other match {
> +                       case olit: TypedLiteral => (olit eq this) || (olit.getLexicalForm
== lexicalForm && olit.getDataType == this.getDataType)
> +                       case ostr: String => ostr == lexicalForm
> +                       case _ => false
> +               }
> +       } 
> +
> +       override def hashCode() = lexicalForm.hashCode()
> equals and hashcode are wrong:
> - hashcode violates the definition for TypedLiterals
> - you shoud never return true when compared to a String as this violates the symmetry
principle mandated by the definition of equals

Ah ok. I was wondering why one could not do that. Pitty, because really they should be equal
:-)

> 
> +
> +       def getDataType = XSD.string
>  }
> 
> 
> @@ -103,21 +110,21 @@ case class EzLiteral(lexicalForm: String
> 
>  @deprecated("Don't use yet other than for trying out this class as it may be merged
with another class or changed dramatically." +
>        " Send feedback to CLEREZZA-510. ")
> -class EasyGraph(val graph: TripleCollection) extends SimpleMGraph(graph) {
> +class EasyGraph(val graph: HashSet[Triple]) extends SimpleMGraph(graph) {
>        val namedBnodes = new HashMap[String,EasyGraphNode]
> 
>        /*
>        * because we can't jump straight to super constructor in Scala we need to
>        * create the collection here
>        **/
> -       def this() = this (new SimpleMGraph())
> +       def this() = this (new HashSet[Triple])
> What's the reason for this change? I think there are fundamental questions how editing
of existing graphs should work. I think copying around the data is bad, but don't see the
motivation for this change

Well what do you suggest? This seemed more efficient, but yes, it does not look that good.


>  
> 
>        def +=(other: Graph) = {
>                if (graph ne other) graph.addAll(other)
>        }
> 
>        def bnode: EasyGraphNode = {
> -               new EasyGraphNode(new BNode(), graph)
> +               new EasyGraphNode(new BNode(), this)
>        }
> 
>        def bnode(name: String): EasyGraphNode = {
> @@ -171,10 +178,10 @@ class EasyGraph(val graph: TripleCollect
>  */
>  class EasyGraphNode(val ref: NonLiteral, val graph: TripleCollection) extends GraphNode(ref,
graph) {
> 
> -       lazy val easyGraph = graph match {
> -               case eg: EasyGraph => eg
> -               case other: TripleCollection => new EasyGraph(graph)
> -       }
> +//     lazy val easyGraph = graph match {
> +//             case eg: EasyGraph => eg
> +//             case other: TripleCollection => new EasyGraph(graph)
> +//     }
> 
>        /*
>         * create an EasyGraphNode from this one where the backing graph is protected
from writes by a new
> 
> Modified: incubator/clerezza/trunk/parent/rdf.scala.utils/src/test/scala/org/apache/clerezza/rdf/scala/utils/EasyGraphTest.scala
> URL: http://svn.apache.org/viewvc/incubator/clerezza/trunk/parent/rdf.scala.utils/src/test/scala/org/apache/clerezza/rdf/scala/utils/EasyGraphTest.scala?rev=1138602&r1=1138601&r2=1138602&view=diff
> ==============================================================================
> --- incubator/clerezza/trunk/parent/rdf.scala.utils/src/test/scala/org/apache/clerezza/rdf/scala/utils/EasyGraphTest.scala
(original)
> +++ incubator/clerezza/trunk/parent/rdf.scala.utils/src/test/scala/org/apache/clerezza/rdf/scala/utils/EasyGraphTest.scala
Wed Jun 22 19:59:49 2011
> @@ -20,10 +20,9 @@ package org.apache.clerezza.rdf.scala.ut
> 
>  import org.apache.clerezza.rdf.utils._
>  import org.apache.clerezza.rdf.ontologies._
> -import Preamble._
> -import org.apache.clerezza.rdf.core._
> -import impl.{TypedLiteralImpl, TripleImpl, SimpleMGraph, PlainLiteralImpl}
>  import org.junit._
> +import org.apache.clerezza.rdf.core._
> +import impl.{TypedLiteralImpl, PlainLiteralImpl, SimpleMGraph, TripleImpl}
> 
>  class EasyGraphTest {
> 
> @@ -64,6 +63,7 @@ class EasyGraphTest {
>                gr.add(new TripleImpl(danny,FOAF.knows, reto))
> 
>                gr.add(new TripleImpl(henry,FOAF.name,new PlainLiteralImpl("Henry Story")))
> +               gr.add(new TripleImpl(henry,FOAF.currentProject,new UriRef("http://webid.info/")))
>                gr.add(new TripleImpl(henry,RDF.`type`, FOAF.Person))
>                gr.add(new TripleImpl(henry,FOAF.knows, danny))
>                gr.add(new TripleImpl(henry,FOAF.knows, reto))
> @@ -77,18 +77,21 @@ class EasyGraphTest {
>        }
> 
>        @Test
> -       def plainChracter {
> -/*
> -               val g = new EasyGraph(simpleMGraph)
> -               val sub = g.bnode
> -               ( g.u("http://bblfish.net/#hjs") a FOAF.Person
> -                has FOAF.name to {"Henry Story"}
> -               )
> -
> -               Assert.assertEquals(tinyGraph, simpleMGraph.getGraph)
> -*/
> +       def testEquality {
> Seems an odd name for the test, the method  name should describe what this testcase is
about, "test" doen't need to be part of the method name (but can)

Well just testing two graph equalities. But yes, don't care about the name really.

>  
> +               val gr = new SimpleMGraph
> +
> +               val reto= new BNode()
> +               gr.add(new TripleImpl(reto,RDF.`type`, FOAF.Person))
> +
> +               val ez = new EasyGraph()
> +               ez.bnode ∈ FOAF.Person
> +
> +               Assert.assertEquals("the two graphs should be of same size",gr.size(),ez.size())
> +               Assert.assertTrue("the two graphs should be equals",gr.getGraph.equals(ez.getGraph))
//mutable graphs cannot be compared for equality
> Its better to use assertEquals comparing the graphs (as in the other tests) as this leads
to more informative failure messages. The comment that "muable graphs cannot be compared for
equality" is wrong, only that mutable graphs are equals just by being isomorphic.

true. I was in a hurry, so when I finally realised I had to compare non-mutable graphs for
equality, I did it this way.

>  
> +
>        }
> 
> +
>        @Test
>        def usingSymbolicArrows {
>                import org.apache.clerezza.rdf.scala.utils.EasyGraph._
> @@ -100,26 +103,29 @@ class EasyGraphTest {
>                         ⟠ FOAF.name ⟶ "Reto Bachman-Gmür".lang(rm)
>                         ⟠ FOAF.title ⟶ "Mr"
>                         ⟠ FOAF.currentProject ⟶ "http://clerezza.org/".uri
> -                        ⟠ FOAF.knows ⟶ (ez.u("http://bblfish.net/#hjs") ∈
FOAF.Person
> +                        ⟠ FOAF.knows ⟶ (
> +                            ez.u("http://bblfish.net/#hjs") ∈ FOAF.Person
>                                  ⟠ FOAF.name ⟶ "Henry Story"
>                                  ⟠ FOAF.currentProject ⟶ "http://webid.info/".uri
> -                                 ⟵ identity ⟞ ( ez.bnode ∈ RSAPublicKey
> +                                 ⟵ identity ⟞ (
> +                                     ez.bnode ∈ RSAPublicKey
>                                         ⟠ modulus ⟶ 65537
>                                         ⟠ public_exponent ⟶ (bblfishModulus^^hex)
// brackets needed due to precedence
>                                  )
>                                  ⟠ FOAF.knows ⟶ ez.bnode("reto")
>                                            ⟠ FOAF.knows ⟶ ez.bnode("danny")
>                         )
> -                        ⟠ FOAF.knows ⟶ (ez.bnode("danny") ∈ FOAF.Person
> -                                 ⟠ FOAF.name ⟶ "Danny Ayers".lang('en)
> +                        ⟠ FOAF.knows ⟶ (
> +                            ez.bnode("danny") ∈ FOAF.Person
> +                                 ⟠ FOAF.name ⟶ "Danny Ayers".lang(en)
>                             ⟠ FOAF.knows ⟶ "http://bblfish.net/#hjs".uri //knows
>                                            ⟠ FOAF.knows ⟶ ez.bnode("reto")
>                         )
>                 )
> -               Assert.assertEquals("Both graphs should have the same size",tinyGraph.size,
ez.size)
> -               Assert.assertEquals("Both graphs should contain exactly the same triples",
tinyGraph, ez.getGraph)
> +               Assert.assertEquals("the two graphs should be of same size",tinyGraph.size(),ez.size())
> +               Assert.assertTrue("Both graphs should contain exactly the same triples",tinyGraph.equals(ez.getGraph))
>                ez.bnode("danny") ⟠  FOAF.name ⟶  "George"
> -               Assert.assertFalse("Added one more triple, so graphs should no longer
be equal",tinyGraph.equals(ez.getGraph))
> +               Assert.assertFalse("Added one more triple, so graphs should no longer
be equal", tinyGraph.equals(ez.getGraph))
>        }
> 
>        @Test
> @@ -133,24 +139,26 @@ class EasyGraphTest {
>                         -- FOAF.name --> "Reto Bachman-Gmür".lang(rm)
>                         -- FOAF.title --> "Mr"
>                         -- FOAF.currentProject --> "http://clerezza.org/".uri
> -                        -- FOAF.knows --> ( ez.u("http://bblfish.net/#hjs").a(FOAF.Person)
> +                        -- FOAF.knows --> (
> +                            ez.u("http://bblfish.net/#hjs").a(FOAF.Person)
>                                  -- FOAF.name --> "Henry Story"
>                                  -- FOAF.currentProject --> "http://webid.info/".uri
> -                                 -<- identity -- (  ez.bnode.a(RSAPublicKey) //.
notation because of precedence of operators
> +                                 -<- identity -- (
> +                                          ez.bnode.a(RSAPublicKey) //. notation because
of precedence of operators
>                                               -- modulus --> 65537
>                                               -- public_exponent --> (bblfishModulus^^hex)
// brackets needed due to precedence
> -                                              )
> +                                          )
>                                  -- FOAF.knows --> ez.bnode("reto")
>                                            -- FOAF.knows --> ez.bnode("danny")
>                         )
>                         -- FOAF.knows --> (ez.bnode("danny").a(FOAF.Person)
> -                                 -- FOAF.name --> "Danny Ayers".lang('en)
> +                                 -- FOAF.name --> "Danny Ayers".lang(en)
>                             -- FOAF.knows --> "http://bblfish.net/#hjs".uri //knows
>                                            -- FOAF.knows --> ez.bnode("reto")
>                         )
>                 )
> -               Assert.assertEquals("Both graphs should have the same size",tinyGraph.size,
ez.size)
> -               Assert.assertEquals("Both graphs should contain exactly the same triples",tinyGraph,
ez.getGraph)
> +               Assert.assertEquals("the two graphs should be of same size",tinyGraph.size(),ez.size())
> +               Assert.assertTrue("Both graphs should contain exactly the same triples",tinyGraph.equals(ez.getGraph))
> Again, why did you change this= assertEquals produces a failure message telling what
is expected and what it effectively got.

No good reason. hurry to get things done. Not a big deal. But thanks for pointing it out.

>  
>                ez.bnode("danny") -- FOAF.name --> "George"
>                Assert.assertFalse("Added one more triple, so graphs should no longer
be equal",tinyGraph.equals(ez.getGraph))
> 
> I think that my proposed name LiteralBuilder may not have been the best,  but that the
concept of having a builder class from which you can get a UriRef of a typed or a languaged
literal is much cleaner than the approach of getting a special typed literal which is (and
has to) be equal to any other typed-literal for the same value but which has some special
methods which the other instances that appear to be equals don't have.

Those are not transformative methods, so it's ok. They don't change the state of the literal,
they just map your literal onto another literal. Many classes have that. Integer, and all
the numbers allows you to map to string, for example.

> As the advantage of directly having an object that can be used as an xsd:string typed
literal can also be achieved with implicit conversion I think the cleaner approach with the
Builder should be preferred (its nice to see that both approaches pass the same test suite).

I frankly find this is much cooler. We have a string literal that can be used immediately
as a normal string literal, but if you wish to map your string literal to a language literal
you have the methods to do it. This seems like a much more functional way of writing things
to me. If we were building this in Scala without the java objects above I'd suggest defining
simple string Literals this way.

I think it is logically and functionally correct, and more efficient. We get rid of a builder
object that would be wasted in cases where we just wanted to create a literal.

Henry

> 
> Cheers,
> Reto
> 

Social Web Architect
http://bblfish.net/


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message