incubator-clerezza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Reto Bachmann-Gmuer <reto.bachm...@trialox.org>
Subject Reviewing svn commit: r1138602 (EasyGraph, Conversions)
Date Wed, 22 Jun 2011 20:57:53 GMT
Hi Henry

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

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


>
> 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

 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

>
>  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


>
>        /**
>         * @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


>
> @@ -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


> +
> +       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


>
>        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)


> +               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.


> +
>        }
>
> +
>        @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.


>                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. 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).

Cheers,
Reto

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