View mode: basic / threaded / horizontal-split · Log in · Help
September 29, 2011
Request for pre-review: std.serialization/orange
I would like to have some form of pre-review of my serialization library 
Orange for later inclusion in Phobos as std.serialization (or similar).

The reason for why I would like to have a pre-review is that Orange, in 
its current state, supports both D1/Tango and D2/Phobos. I don't want to 
spend a lot of time in interacting Orange with Phobos if it have no 
chance of getting accepted into Phobos.

I'm hoping with can do like a regular review with the assumption that if 
it gets accepted into Phobos I will remove all D1/Tango specific code, 
integrate it properly with Phobos and do a second regular review.

A couple of notes for the review:

* The most important packages are: orange.serialization and 
orange.serialization.archives

* I will not accept having just one module for this library. I'm hoping 
to have a package called "std.serialization" or similar. The question is 
if there needs to be a (sub)package for the archives as well.

* For the unit tests I've used my own kind of micro unit test framework 
(that is included). Is that something we want to have in general in 
Phobos so other modules can take advantage of that? Or should I just rip 
out the framework?

* The unit tests are located in its own package, I'm not very happy 
about putting the unit tests in the same module as the rest of the code, 
i.e. the serialization module

* I'm using some utility functions located in the "util" and "core" 
packages, what should we do about those, where to put them?

* I'm not using all of the functions/modules in the above mentioned 
utility packages

For usage examples, see the project page: 
http://dsource.org/projects/orange/wiki/Tutorials

For more extended usage examples, see the unit tests: 
https://github.com/jacob-carlborg/orange/tree/master/tests

Sources: https://github.com/jacob-carlborg/orange
Project page: http://dsource.org/projects/orange/
Documentation: 
http://dl.dropbox.com/u/18386187/orange_docs/orange.serialization.Serializer.html

(Don't forget clicking the "Package" tab in the top corner to see the 
documentation for the rest of the modules)

-- 
/Jacob Carlborg
September 29, 2011
Re: Request for pre-review: std.serialization/orange
> * For the unit tests I've used my own kind of micro unit test framework
> (that is included). Is that something we want to have in general in
> Phobos so other modules can take advantage of that? 

There should be something in Phobos. This should involve two steps.

First define a protocol, so that multiple unittest libraries can
be used in one project (i.e a library uses A and another B). Than 
write a library on top of this.

My suggestion for a protocol would be something simple as:

1. There should only be one approach to unittest used per module
2. If you replace the test runner, call the old one first.
September 30, 2011
Re: Request for pre-review: std.serialization/orange
On 2011-09-29 21:42, Tobias Pankrath wrote:
>
>> * For the unit tests I've used my own kind of micro unit test framework
>> (that is included). Is that something we want to have in general in
>> Phobos so other modules can take advantage of that?
>
> There should be something in Phobos. This should involve two steps.
>
> First define a protocol, so that multiple unittest libraries can
> be used in one project (i.e a library uses A and another B). Than
> write a library on top of this.
>
> My suggestion for a protocol would be something simple as:
>
> 1. There should only be one approach to unittest used per module
> 2. If you replace the test runner, call the old one first.

My unit test framework does two things, provides a some form of context 
for the tests; and it catches all assert exceptions to let other tests 
continue. It also gives a quite nice report of what tests failed.

-- 
/Jacob Carlborg
September 30, 2011
Re: Request for pre-review: std.serialization/orange
On Thursday, September 29, 2011 20:58:30 Jacob Carlborg wrote:
> * For the unit tests I've used my own kind of micro unit test framework
> (that is included). Is that something we want to have in general in
> Phobos so other modules can take advantage of that? Or should I just rip
> out the framework?

I very much doubt that it would be accepted. assertPred failed to make it in 
spite of the various benefits that it provided, and a number of folks seem to 
be against more complicated unit testing features making it into Phobos. Not 
to mention, there's already some discussion of the unit tests taking too long. 
We aren't going to want anything that adds yet more overhead.

> * The unit tests are located in its own package, I'm not very happy
> about putting the unit tests in the same module as the rest of the code,
> i.e. the serialization module

Well, that's the way that Phobos does it, and it's essentially how D's unit 
tests are intended to work (though obviously, you can work around that if you 
really want to). Phobos' makefiles are set up with the idea that each module's 
unit tests are included in that module. I'm not sure that it's guaranteed that 
we wouldn't allow the unit tests to be separate, but that's not how any of the 
other Phobos unit tests works. And personally, I find that the unit tests are 
far more maintainable when they're right next to the functions that they test.

- Jonathan M Davis
September 30, 2011
Re: Request for pre-review: std.serialization/orange
On Thu, 29 Sep 2011 14:58:30 -0400, Jacob Carlborg <doob@me.com> wrote:
> I would like to have some form of pre-review of my serialization library
> Orange for later inclusion in Phobos as std.serialization (or similar).

[snip]

This is a quick note on the API design; I'm planning on doing a deeper review of the code + API later.

Re: registerSerializer
Type.stringof is not unique and can't be used by your serializer. Furthermore, allowing users to manually set the lookup string is going to be a major source of silent errors / bugs and exposes a large portions of your internals. Doing it in this way prevents you from updating how the back-end looks up types.

I'd recommend using: typeid(A).toString internally instead as this is unique and registerSerializer's API to

void registerSerializer(Derived,Base)(void delegate(Base) dg) if( is(Derived:Base) ) {}

which would be called via:

registerSerializer!Foo(dg);


The method should also be static: If I'm registering a custom serialization method, I don't want to duplicate that mapping everywhere a serializer is instanced. I don't even want to duplicate it for every type of serializer. I think there needs to be some granularity for this: i.e. instance -> type -> global.

Also, repeat the above for registerDeserializer.


Re: deserialize some of the example don't look like they're correct.
September 30, 2011
Re: Request for pre-review: std.serialization/orange
On 2011-09-30 08:39, Jonathan M Davis wrote:
> On Thursday, September 29, 2011 20:58:30 Jacob Carlborg wrote:
>> * For the unit tests I've used my own kind of micro unit test framework
>> (that is included). Is that something we want to have in general in
>> Phobos so other modules can take advantage of that? Or should I just rip
>> out the framework?
>
> I very much doubt that it would be accepted. assertPred failed to make it in
> spite of the various benefits that it provided, and a number of folks seem to
> be against more complicated unit testing features making it into Phobos. Not
> to mention, there's already some discussion of the unit tests taking too long.
> We aren't going to want anything that adds yet more overhead.

That's why I'm asking. BTW, if we had a proper unit test framework it 
could be possible to run a smaller set of unit tests to keep the running 
time down to a minimum (not something my framework can do).

>> * The unit tests are located in its own package, I'm not very happy
>> about putting the unit tests in the same module as the rest of the code,
>> i.e. the serialization module
>
> Well, that's the way that Phobos does it, and it's essentially how D's unit
> tests are intended to work (though obviously, you can work around that if you
> really want to). Phobos' makefiles are set up with the idea that each module's
> unit tests are included in that module. I'm not sure that it's guaranteed that
> we wouldn't allow the unit tests to be separate, but that's not how any of the
> other Phobos unit tests works. And personally, I find that the unit tests are
> far more maintainable when they're right next to the functions that they test.
>
> - Jonathan M Davis

The thing is that just to do a simple test, like serializing an int, it 
requires some extra code. It's not like calling a single function. It 
requires:

* Something to serialize
* A serializer
* An archive
* Actually doing the serialization
* Performing several checks on the serialized data

BTW I don't think it scales to have the unit tests in the same module if 
you want to perform extensive unit tests or if it's a complicated module.

The unit test code should be equally well organized, readable and 
maintainable as the regular code. I think keeping it in the same module 
makes that harder.

-- 
/Jacob Carlborg
September 30, 2011
Re: Request for pre-review: std.serialization/orange
On 2011-09-30 15:03, Robert Jacques wrote:
> On Thu, 29 Sep 2011 14:58:30 -0400, Jacob Carlborg <doob@me.com> wrote:
>> I would like to have some form of pre-review of my serialization library
>> Orange for later inclusion in Phobos as std.serialization (or similar).
>
> [snip]
>
> This is a quick note on the API design; I'm planning on doing a deeper
> review of the code + API later.
>
> Re: registerSerializer
> Type.stringof is not unique and can't be used by your serializer.

I don't know why but I thought Type.stringof == typeid(Type).toString 
for everything that wasn't a class.

> Furthermore, allowing users to manually set the lookup string is going
> to be a major source of silent errors / bugs and exposes a large
> portions of your internals. Doing it in this way prevents you from
> updating how the back-end looks up types.
>
> I'd recommend using: typeid(A).toString internally instead as this is
> unique and registerSerializer's API to
>
> void registerSerializer(Derived,Base)(void delegate(Base) dg) if(
> is(Derived:Base) ) {}
>
> which would be called via:
>
> registerSerializer!Foo(dg);

Didn't thought of that, thanks.

> The method should also be static: If I'm registering a custom
> serialization method, I don't want to duplicate that mapping everywhere
> a serializer is instanced. I don't even want to duplicate it for every
> type of serializer. I think there needs to be some granularity for this:
> i.e. instance -> type -> global.

"register" is static, "registerSerializer" is not because I'm not 
entirely sure how I want the API to behave.

What if I want to serialize a class in two different places. In one 
place I want to serialize it by default and in the other I want to do 
custom serialization?

"I don't even want to duplicate it for every type of serializer". I'm 
not sure I quite understand, there's only one type of serializer.

> Also, repeat the above for registerDeserializer.
>
>
> Re: deserialize some of the example don't look like they're correct.

They look correct to me. Note that two of the three "deserialize" 
methods should only be called when performing custom deserialization of 
a class/strut. This method will then, most likely, be called from within 
the class to manually deserialize the fields.

-- 
/Jacob Carlborg
September 30, 2011
Re: Request for pre-review: std.serialization/orange
On Friday, September 30, 2011 16:25:47 Jacob Carlborg wrote:
> On 2011-09-30 08:39, Jonathan M Davis wrote:
> > On Thursday, September 29, 2011 20:58:30 Jacob Carlborg wrote:
> >> * For the unit tests I've used my own kind of micro unit test
> >> framework
> >> (that is included). Is that something we want to have in general in
> >> Phobos so other modules can take advantage of that? Or should I just
> >> rip
> >> out the framework?
> > 
> > I very much doubt that it would be accepted. assertPred failed to make
> > it in spite of the various benefits that it provided, and a number of
> > folks seem to be against more complicated unit testing features making
> > it into Phobos. Not to mention, there's already some discussion of the
> > unit tests taking too long. We aren't going to want anything that adds
> > yet more overhead.
> 
> That's why I'm asking. BTW, if we had a proper unit test framework it
> could be possible to run a smaller set of unit tests to keep the running
> time down to a minimum (not something my framework can do).

That can be done easily enough with a version block with a version specific to 
running longer or shorter tests. No additional framework is necessary.  Don 
has already brought it up in the Phobos newsgroup, but it's unclear whether 
we're going to do anything about it. The main problem is the compile time, not 
the run time, so it's ultimately a compiler issue. Templates and CTFE in 
particular make the whole thing slower.

> >> * The unit tests are located in its own package, I'm not very happy
> >> about putting the unit tests in the same module as the rest of the
> >> code,
> >> i.e. the serialization module
> > 
> > Well, that's the way that Phobos does it, and it's essentially how D's
> > unit tests are intended to work (though obviously, you can work around
> > that if you really want to). Phobos' makefiles are set up with the idea
> > that each module's unit tests are included in that module. I'm not sure
> > that it's guaranteed that we wouldn't allow the unit tests to be
> > separate, but that's not how any of the other Phobos unit tests works.
> > And personally, I find that the unit tests are far more maintainable
> > when they're right next to the functions that they test.
> > 
> > - Jonathan M Davis
> 
> The thing is that just to do a simple test, like serializing an int, it
> requires some extra code. It's not like calling a single function. It
> requires:
> 
> * Something to serialize
> * A serializer
> * An archive
> * Actually doing the serialization
> * Performing several checks on the serialized data
> 
> BTW I don't think it scales to have the unit tests in the same module if
> you want to perform extensive unit tests or if it's a complicated module.
> 
> The unit test code should be equally well organized, readable and
> maintainable as the regular code. I think keeping it in the same module
> makes that harder.s

In my experience, it's _much_ easier to have the tests right next to what 
they're testing. I've found it _very_ annoying when I've had to put unit tests 
after a templated type, because the unit tests coludn't be _in_ the templated 
type where they would be instantiated with it. The Interval unit tests in 
std.datetime are way less manageable because of that.

Now, if you have unit testing each function individually doesn't work very 
well, and if you have to do a bunch of setup for any testing, then the 
situation is a bit different, but you could probably just stick all of the 
tests at the end of the file then, where they're then separate from the code.

- Jonathan M Davis
September 30, 2011
Re: Request for pre-review: std.serialization/orange
On 2011-09-30 18:09, Jonathan M Davis wrote:
> That can be done easily enough with a version block with a version specific to
> running longer or shorter tests. No additional framework is necessary.  Don
> has already brought it up in the Phobos newsgroup, but it's unclear whether
> we're going to do anything about it. The main problem is the compile time, not
> the run time, so it's ultimately a compiler issue. Templates and CTFE in
> particular make the whole thing slower.

Ok, I see.

Phobos is like Boost, everything is templates, that's what we get. 
Compiling my unit tests with D1 takes about 0.7 seconds, using D2 it 
takes about 1.4 seconds.

> Now, if you have unit testing each function individually doesn't work very
> well, and if you have to do a bunch of setup for any testing, then the
> situation is a bit different, but you could probably just stick all of the
> tests at the end of the file then, where they're then separate from the code.
>
> - Jonathan M Davis

I had all the unit tests in one module at first, but I thought it was 
too much to have in one module and now I have several more unit tests.

-- 
/Jacob Carlborg
October 01, 2011
Re: Request for pre-review: std.serialization/orange
On Fri, 30 Sep 2011 10:41:48 -0400, Jacob Carlborg <doob@me.com> wrote:
> On 2011-09-30 15:03, Robert Jacques wrote:
>> On Thu, 29 Sep 2011 14:58:30 -0400, Jacob Carlborg <doob@me.com> wrote:

[snip]

>> The method should also be static: If I'm registering a custom
>> serialization method, I don't want to duplicate that mapping everywhere
>> a serializer is instanced. I don't even want to duplicate it for every
>> type of serializer. I think there needs to be some granularity for this:
>> i.e. instance -> type -> global.
>
> "register" is static, "registerSerializer" is not because I'm not
> entirely sure how I want the API to behave.
>
> What if I want to serialize a class in two different places. In one
> place I want to serialize it by default and in the other I want to do
> custom serialization?

I agree, which is why I suggested lookup should have some granuality. i.e. that there is both a global store of serialization methods and a per instance store of serialization methods. Lookup would first look in the local store before defaulting to the global store. But this should be a separate pair of functions.

> "I don't even want to duplicate it for every type of serializer". I'm
> not sure I quite understand, there's only one type of serializer.

I'm sorry, I was thinking about archive types (i.e. JSON vs XML) and somehow thinking that the Serializers would be different for each. (I was thinking that the serializer was templated on the archive for some reason.)

>> Also, repeat the above for registerDeserializer.
>>
>>
>> Re: deserialize some of the example don't look like they're correct.
>
> They look correct to me. Note that two of the three "deserialize"
> methods should only be called when performing custom deserialization of
> a class/strut. This method will then, most likely, be called from within
> the class to manually deserialize the fields.
>

Both

T deserialize  (T)();
T deserialize  (T)(string key);

have the following example:

 class Foo
 {
 	int a;

 	void fromData (Serializer serializer, Serializer.Data key)
 	{
 		a = serializer!(int)("a");
 	}
 }
« First   ‹ Prev
1 2 3
Top | Discussion index | About this forum | D home