On 2013-06-10 23:40, Jonas Drewsen wrote:
A quick first look for now:
In general I think that you should clone phobos and merge orange into
std.serialize in order for us to see how it really fits into phobos.
As such I think it feels more like a RFC than formal review because it
couldn't possible go into phobos in its current state even if we ignored
all comments from the this list.
Ok. What's already clear:
* orange.serialization.Events -> std.serialization.events
* orange.serialization.RegisterWrapper -> std.serialization.registerwrapper
* orange.serialization.Serializable -> std.serialization.serializable
* orange.serialization.SerializationException ->
std.serialization.serializationexception
* orange.serialization.Serializer -> std.serialization.serializer
* orange.serialization.archives.Archive ->
std.serialization.archives.archive
* orange.serialization.archives.XmlArchive ->
orange.serialization.archives.xmlarchive
* orange.xml.PhobosXml -> (merge with) std.xml
Now for something more constructive :)
* I'm using some utility functions located in the "util" and "core"
packages, what should we do about those, where to put them?
The core package only have the core.Attribute module which defines a
meta attribute which is a new concept in phobos. I guess that should
either be removed or we should spawn a discussion about if we want such
a thing or not. Is it possible to do without it?
Yes, it's possible to do without. But I rather like to start a
discussion. I'll create a new thread for this.
The util package
----------------
util.use.d : I think the Use struct feels a bit like abusing the 'in'
operator to work around D not supporting blocks or something else that
would provide the desired syntax. Personally I think it should go.
Yes, it is. I can move util.Use.restore into XmlArchive and change it to
use a template delegate instead of using "in".
util.traits.d : Most of them should go to std.traits or use something
from std.traits instead if possible
Yes. Some can probably removed.
util.reflection.d : Should probably be part if std.traits as well since
there are already some reflection code in there. I guess std.traits
should make use of the new package.d thing to split up the module into
several files.
util.ctfe.d : Wouldn't now where to put it.
I could either move that inline where it's used or remove it.
util.collection.array : should probably go into std.exception
Yes.
It seems all the module filenames are camel cased. They should be all
lowercase.
Yes, see above.
The _.d modules should probably be renamed to package.d now.
Yes, that was like three commits ago :)
IMHO I think the tests would fit nicely into the package itself. Close
to the code it tests.
Do you mean in package.d? Yes, if that's possible.
Could you provide the docs formatted as it would be in dlang.org since
only that way it is also possible to review formatting.
Ok, I'll see if I can figure it out.
Keep up the good work. I for one are really looking forward to finally
getting this thing in.
Cool. Thanks for the review.
--
/Jacob Carlborg
