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: Strange commit (was svn commit: r1101934)
Date Wed, 11 May 2011 19:37:50 GMT
Ok, did two small commits to undo most of what I had commited.
Hopefully just the good stuff remains now.

On 11 May 2011, at 20:36, Reto Bachmann-Gmuer wrote:

> On Wed, May 11, 2011 at 7:43 PM, Henry Story <henry.story@bblfish.net> wrote:
>>> EasyGraph seems completly unrelated to CLEREZZA-511
>> 
>> Well not quite. EasyGraph has automatic conversations which are listed in
>> CLEREZZA-511. Here a some
>> 
>> implicit def string2lit(str: String) = new PlainLiteralScala(str)
>> implicit def date2lit(date: Date) = DATE_CONVERTER.createTypedLiteral(date)
>> implicit def int2lit(int: Int) = INTEGER_CONVERTER.createTypedLiteral(int)
>> 
>> Now of course, since I asked by opening the issue and you preferred not to
>> use the converters directly, I have followed your advice and not used those directly,
>> but the method call instead. Seems like the discussion was fruitful.

<sarcasm>
> that's handy so we can now associate everything that use the
> literalfactory to ZZ-511 as it would potentially be different if
> ZZ-511 had been resolved differently. I suggest we rename the issue to
> "Make the world a better place" only slightly broadening the scope.
> (do we need to add sarcasm tags on this list?)
</sarcasm>

There you go :-) Looks better already.


>>> 
>>> For the rest of the commit I don't see what usefull change you think
>>> you made.
>> 
>> below you agree some where made.
> 
> Yes, eventually I discovered something I'm could agree with. I didn't
> *see" it at the time I wrote the sentence above. This shows the
> importance of having tiny patches associated to well defined issues
> that can rapidly be closed. In a small patch for an issue labeled
> "improve perfomance of boolean to literal back-conversions" i would
> have had less troubles finding and understanding the possible
> improvement.

I agree that patch was not so good really. So I undid the bad parts.
Hopefully just the good parts are left. 


> 
>> 
>>> I see some code rearrengement and introduction of private
>>> fields with imho don't make things more readable but harder to
>>> maintain. I see that you added questions which I think belong on the
>>> mailing list and not in the code, some lobbying for the modularized
>>> literal factory and yes, I think I could agree with the TRUE/FALSE
>>> constants.
>> 
>> Ok. So that's useful then.
>> 
>>> I see no reason to make TypeConverter public.
>> 
>> Well it is quite easy to see that there are going to be a lot more type
>> conversions  than you can list in your code. One can come up with
>> new literal formats at the drop of a hat. So clearly people should be able
>> to open add new TypeConverters.
> 
> - They are not because there is no public way to add them.
> - Public members should have Javadoc comments
> - This is in scope of ZZ-423

agree. Will open issue when I need it, as I say below in fact.

> 
>> 
>> But ok, one can open that in a new issue. And indeed I thought I had left
>> it private. I'll make it private again for the moment.
>> 
>>> 
>>> Please:
>>> - separate the patches for code-renicing from actual improvements
>>> (here the performance improvement)
>>> - associate patches to an issue from which the motivation for the
>>> patch can be deducted
>>> - for performance improvement: focus on bottle-necks that show up in
>>> the profiler, otherwise we might make the code no complicates without
>>> actual gain
>>> - post questions to the mailing list
>> 
>> I have not  found the list very responsive usually.
> I'm sorry you got this impression. I think the best changes for
> responses for questions like the one you added to the code are if you
> keep the question short and post a link to the respective code section
> on http://svn.apache.org/viewvc/, ideally to the patch the questioned
> section was introduced (so one can go back to the issue which makes it
> easier to understand, as long as its not the
> make-the-world-a-better-place issue)

funny but that is exactly what I did in this 511 post. You'll notice I posted a
patch, and left it up for discussion. 

I just felt afterwads that some of the work I had written could be useful later 
perhaps. But  ok, not a bit deal.

> 
>> Bug reports seem to work better for a conversation.
> Tha's fine too, there is the issue type "question" which might be usefull.

But also we don't want to open thousand of bug reports, or else people will think
the project is going nowhere. That is why sometimes I think it is good to put
comments in the code, as thoughts. Those are pointers for future reports 
that in a good IDE one can find by searching for todo: comments.

> 
>> As you saw recently even posting attachements to the list is
>> difficult.
> Attachments were never supported on the mailing list. I think all
> apache mailing lists are configured that way. I just added a note to
> the mailing paste on the website (takes a few hours to be live).

Good. No problem with that policy. It's just that the issue database has
been the way we have been communicating more successfully.

> 
>> 
>>> - if adding todos to the code refer to the existing issue that would
>>> cover them, the "todo: create a subclass of TypedLiteral that contains
>>> the original value, then one would not need" could refer to
>>> CLEREZZA-423
>> 
>> Sounds like a good idea. I think it is good to have pointers going both ways.
>> 
>>> 
>>> I'm sorry for being pedantic, but I think that clerezza can only be a
>>> stable and manageable code base if we stick to the process and are a
>>> bit conservative on which patches we accept.
>> 
>> As long as we don't end up in a process nightmare. So of this patch I think
>> most of it is ok, just the public constructor is not. I'll fix that.
> yeah well, the bunch of private constants all used in exactly one
> place is not at improvement in my view but its a detail.

yes, I had just written that code, and I did not want to throw it away.
Was not really worth it though.

> 
>> 
>> Thanks for reviewing.
> you're welcome :)
> 
> To your question what "Collections.addAll(decimalTypes, xsdInteger,
> xsdInt, xsdByte, xsdShort);" is for: this adds the literal types that
> can be converted to various java classes representing numbers to the
> collection decimalTypes, this collection is used in various converters
> to check if they are capable of converting the literal at hand.

Ah, silly me. I had not paid attention to the addAll method. I always
use those methods to create collections, so I assumed it returned a 
anonymous collection and I could not see why it was doing that.


> 
> Reto
> 
>> 
>> Henry
>> 
>>> 
>>> Cheers,
>>> Reto
>>> 
>>> 
>>> 
>>> 
>>> On Wed, May 11, 2011 at 6:04 PM,  <bblfish@apache.org> wrote:
>>>> Author: bblfish
>>>> Date: Wed May 11 16:04:20 2011
>>>> New Revision: 1101934
>>>> 
>>>> URL: http://svn.apache.org/viewvc?rev=1101934&view=rev
>>>> Log:
>>>> CLEREZZA-511: Was going to Allow access to single individual SimpleLiteralFactory
converters but reto prefers not. Other changes are still useful
>>>> 
>>>> Modified:
>>>>    incubator/clerezza/trunk/parent/rdf.core/src/main/java/org/apache/clerezza/rdf/core/impl/SimpleLiteralFactory.java
>>>>    incubator/clerezza/trunk/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EasyGraph.scala
>>>> 
>>>> Modified: incubator/clerezza/trunk/parent/rdf.core/src/main/java/org/apache/clerezza/rdf/core/impl/SimpleLiteralFactory.java
>>>> URL: http://svn.apache.org/viewvc/incubator/clerezza/trunk/parent/rdf.core/src/main/java/org/apache/clerezza/rdf/core/impl/SimpleLiteralFactory.java?rev=1101934&r1=1101933&r2=1101934&view=diff
>>>> ==============================================================================
>>>> --- incubator/clerezza/trunk/parent/rdf.core/src/main/java/org/apache/clerezza/rdf/core/impl/SimpleLiteralFactory.java
(original)
>>>> +++ incubator/clerezza/trunk/parent/rdf.core/src/main/java/org/apache/clerezza/rdf/core/impl/SimpleLiteralFactory.java
Wed May 11 16:04:20 2011
>>>> @@ -55,8 +55,36 @@ public class SimpleLiteralFactory extend
>>>> 
>>>> 
>>>>        final private static Set<UriRef> decimalTypes = new HashSet<UriRef>();
>>>> +
>>>> +       final static private TypeConverter<byte[]> BYTE_ARRAY_CONVERTER
= new ByteArrayConverter();
>>>> +       final static private TypeConverter<Boolean> BOOLEAN_CONVERTER
= new BooleanConverter();
>>>> +       final static private TypeConverter<Date> DATE_CONVERTER = new
DateConverter();
>>>> +       final static private TypeConverter<String> STRING_CONVERTER
= new StringConverter();
>>>> +       final static private TypeConverter<Integer> INTEGER_CONVERTER
= new IntegerConverter();
>>>> +       final static private TypeConverter<BigInteger> BIG_INTEGER_CONVERTER
= new BigIntegerConverter();
>>>> +       final static private TypeConverter<Long> LONG_CONVERTER = new
LongConverter();
>>>> +       final static private TypeConverter<Double> DOUBLE_CONVERTER
= new DoubleConverter();
>>>> +       final static private TypeConverter<UriRef> URIREF_CONVERTER
= new UriRefConverter();
>>>> +
>>>> +       final private static Map<Class<?>, TypeConverter<?>>
typeConverterMap = new HashMap<Class<?>, TypeConverter<?>>();
>>>> +       final static Class<? extends byte[]> byteArrayType;
>>>> +
>>>>        static {
>>>> +               //what's this for?
>>>>                Collections.addAll(decimalTypes, xsdInteger, xsdInt, xsdByte,
xsdShort);
>>>> +
>>>> +
>>>> +               byte[] byteArray = new byte[0];
>>>> +               byteArrayType = byteArray.getClass();
>>>> +               typeConverterMap.put(byteArrayType, BYTE_ARRAY_CONVERTER);
>>>> +               typeConverterMap.put(Date.class, DATE_CONVERTER);
>>>> +               typeConverterMap.put(Boolean.class, BOOLEAN_CONVERTER);
>>>> +               typeConverterMap.put(String.class, STRING_CONVERTER);
>>>> +               typeConverterMap.put(Integer.class, INTEGER_CONVERTER);
>>>> +               typeConverterMap.put(BigInteger.class, BIG_INTEGER_CONVERTER);
>>>> +               typeConverterMap.put(Long.class, LONG_CONVERTER);
>>>> +               typeConverterMap.put(Double.class, DOUBLE_CONVERTER);
>>>> +               typeConverterMap.put(UriRef.class, URIREF_CONVERTER);
>>>>        }
>>>> 
>>>>        final private static UriRef xsdDouble =
>>>> @@ -64,13 +92,8 @@ public class SimpleLiteralFactory extend
>>>>        final private static UriRef xsdAnyURI =
>>>>                        new UriRef("http://www.w3.org/2001/XMLSchema#anyURI");
>>>> 
>>>> -       final static Class<? extends byte[]> byteArrayType;
>>>> -       static {
>>>> -               byte[] byteArray = new byte[0];
>>>> -               byteArrayType = byteArray.getClass();
>>>> -       }
>>>> 
>>>> -       private static interface TypeConverter<T> {
>>>> +       public static interface TypeConverter<T> {
>>>>                TypedLiteral createTypedLiteral(T value);
>>>>                T createObject(TypedLiteral literal);
>>>>        }
>>>> @@ -125,15 +148,22 @@ public class SimpleLiteralFactory extend
>>>> 
>>>>                private static final UriRef booleanUri =
>>>>                        new UriRef("http://www.w3.org/2001/XMLSchema#boolean");
>>>> +               public static final TypedLiteralImpl TRUE = new TypedLiteralImpl("true",
booleanUri);
>>>> +               public static final TypedLiteralImpl FALSE = new TypedLiteralImpl("false",
booleanUri);
>>>> 
>>>>                @Override
>>>>                public TypedLiteral createTypedLiteral(Boolean value) {
>>>> -                       return new TypedLiteralImpl(value.toString(), booleanUri);
>>>> +                       if (value) return TRUE;
>>>> +                       else return FALSE;
>>>>                }
>>>> 
>>>> +               //todo: create a subclass of TypedLiteral that contains the
original value, then one would not need
>>>> +               //to do these conversions
>>>>                @Override
>>>>                public Boolean createObject(TypedLiteral literal) {
>>>> -                       if (!literal.getDataType().equals(booleanUri)) {
>>>> +                       if (literal == TRUE) return true;
>>>> +                       else if (literal == FALSE) return false;
>>>> +                       else if (!literal.getDataType().equals(booleanUri))
{
>>>>                                throw new InvalidLiteralTypeException(Boolean.class,
literal.getDataType());
>>>>                        }
>>>>                        return Boolean.valueOf(literal.getLexicalForm());
>>>> @@ -248,20 +278,6 @@ public class SimpleLiteralFactory extend
>>>>                }
>>>>        }
>>>> 
>>>> -       final private static Map<Class<?>, TypeConverter<?>>
typeConverterMap = new HashMap<Class<?>, TypeConverter<?>>();
>>>> -
>>>> -       static {
>>>> -               typeConverterMap.put(byteArrayType, new ByteArrayConverter());
>>>> -               typeConverterMap.put(Date.class, new DateConverter());
>>>> -               typeConverterMap.put(Boolean.class, new BooleanConverter());
>>>> -               typeConverterMap.put(String.class, new StringConverter());
>>>> -               typeConverterMap.put(Integer.class, new IntegerConverter());
>>>> -               typeConverterMap.put(BigInteger.class, new BigIntegerConverter());
>>>> -               typeConverterMap.put(Long.class, new LongConverter());
>>>> -               typeConverterMap.put(Double.class, new DoubleConverter());
>>>> -               typeConverterMap.put(UriRef.class, new UriRefConverter());
>>>> -       }
>>>> -
>>>> 
>>>>        @SuppressWarnings("unchecked")
>>>>        @Override
>>>> 
>>>> 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=1101934&r1=1101933&r2=1101934&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 May 11 16:04:20 2011
>>>> @@ -24,8 +24,10 @@ import collection.mutable.Queue
>>>>  import impl._
>>>>  import org.apache.clerezza.rdf.ontologies.{RDF, RDFS, FOAF}
>>>>  import java.math.BigInteger
>>>> -import java.util.Date
>>>>  import org.apache.clerezza.rdf.utils.{UnionMGraph, GraphNode}
>>>> +import java.util.{HashSet, Collections, Date}
>>>> +import java.lang.Boolean
>>>> +import com.sun.tools.internal.xjc.reader.xmlschema.BindGreen
>>>> 
>>>>  object EasyGraph {
>>>>        final val en = "en"
>>>> @@ -33,10 +35,17 @@ object EasyGraph {
>>>>        final val fr = "fr"
>>>>        val litFactory = new SimpleLiteralFactory()
>>>> 
>>>> +       import org.apache.clerezza.rdf.core.impl.SimpleLiteralFactory._
>>>> +
>>>>        implicit def string2lit(str: String) = new PlainLiteralScala(str)
>>>>        implicit def date2lit(date: Date) = litFactory.createTypedLiteral(date)
>>>> -       implicit def int2lit(date: Int) = litFactory.createTypedLiteral(date)
>>>> -
>>>> +       implicit def int2lit(int: Int) = litFactory.createTypedLiteral(int)
>>>> +       implicit def bigint2lit(bint: BigInt) = litFactory.createTypedLiteral(bint.underlying())
>>>> +       implicit def bigint2lit(bigInt: BigInteger) = litFactory.createTypedLiteral(bigInt)
>>>> +       implicit def bool2lit(boolean: Boolean) = litFactory.createTypedLiteral(boolean)
>>>> +       implicit def scalaBool2lit(boolean: scala.Boolean) = litFactory.createTypedLiteral(boolean)
>>>> +       implicit def long2lit(long: Long) = litFactory.createTypedLiteral(long)
>>>> +       implicit def double2lit(double: Double) = litFactory.createTypedLiteral(double)
>>>> 
>>>> 
>>>>  //     val g = new EasyGraph(new SimpleMGraph)
>>>> @@ -103,6 +112,12 @@ class PlainLiteralScala(string: String)
>>>> 
>>>>  class EasyGraph(val graph: TripleCollection) extends SimpleMGraph(graph)
{
>>>> 
>>>> +  /*
>>>> +       * because we can't jump straight to super constructor in Scala we
need to
>>>> +       * create the collection here
>>>> +       **/
>>>> +       def this() = this( new SimpleMGraph() )
>>>> +
>>>>        def +=(other: Graph) = {
>>>>                  if (graph ne  other) graph.addAll(other)
>>>>        }
>>>> @@ -115,6 +130,9 @@ class EasyGraph(val graph: TripleCollect
>>>> 
>>>>        def apply(subj: NonLiteral) = new EasyGraphNode(subj, this)
>>>> 
>>>> +       //note one could have an apply for a Literal that would return a
InversePredicate
>>>> +       //but that would require restructuring EasyGraphNode so that one
can have an EasyGraphNode
>>>> +       //with a missing ref, or perhaps a sublcass of EasyGraphnode that
only has the <- operator available
>>>>  }
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>> 
>> 
>> Social Web Architect
>> http://bblfish.net/
>> 
>> 

Social Web Architect
http://bblfish.net/


Mime
View raw message