On Wed, May 11, 2011 at 7:43 PM, Henry Story <[email protected]> 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. 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?)
>> >> 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 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 > > 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) > Bug reports seem to work better for a conversation. Tha's fine too, there is the issue type "question" which might be usefull. > 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). > >> - 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. > > 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. Reto > > Henry > >> >> Cheers, >> Reto >> >> >> >> >> On Wed, May 11, 2011 at 6:04 PM, <[email protected]> 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/ > >
