Stepping up to act as a Review Manager for Jacob Carlborg std.serialization

---- Input ----

Code: https://github.com/jacob-carlborg/phobos/tree/serialization

Documentation: https://dl.dropboxusercontent.com/u/18386187/docs/std.serialization/index.html

Previous review thread: http://forum.dlang.org/thread/adyanbsdsxsfdpvoozne@forum.dlang.org

---- Changes since last review ----

- Sources has been integrated into Phobos source tree
- DDOC documentation has been provided in a form it should look like on dlang.org
- Most utility functions/template code depends on have been inlined. Remaining `package` utility modules:
    * std.serialization.archives.xmldocument
    * std.serialization.attribute
    * std.serialization.registerwrapper

---- Information for reviewers ----

Goal of this thread is to detect if there are any outstanding issues that need to fixed before formal "yes"/"no" voting happens. If no critical objections will arise, voting will begin starting with a next week.

Please take this seriously: "If you identify problems along the way, please note if they are minor, serious, or showstoppers." (http://wiki.dlang.org/Review/Process). This information later will be used to determine if library is ready for voting.

If there are any frequent Phobos contributors / core developers please pay extra attention to submission code style and fitting into overall Phobos guidelines and structure.

-------------------------------------

Let the thread begin.

Jacob, it is probably worth creating a pull request with latest rebased version of your proposal to simplify getting a quick overview of changes. Also please tell if there is anything you want/need to implement before merging.

On 2013-08-12 15:27, Dicebot wrote:

> Jacob, it is probably worth creating a pull request with latest rebased
> version of your proposal to simplify getting a quick overview of
> changes.

I don't think a pull request should be made before a module has gone through the review queue and is approved. With Github it's easy to diff between a fork and upstream: https://github.com/jacob-carlborg/phobos/compare/serialization

> Also please tell if there is anything you want/need to implement before merging.

* I have to double check that I haven't added any improvements to Orange not present in std.serialization. But that would only be bug fixes and no public API change so that shouldn't hold the review.

* I forgot to add that the unit tests are, a bit controversial, located in std.serialization.tests

-- 
/Jacob Carlborg

On Monday, 12 August 2013 at 19:55:01 UTC, Jacob Carlborg wrote:
> I don't think a pull request should be made before a module has gone through the review queue and is approved. With Github it's easy to diff between a fork and upstream: https://github.com/jacob-carlborg/phobos/compare/serialization

Agreed.

> * I forgot to add that the unit tests are, a bit controversial, located in std.serialization.tests

That is something that requires the input from Phobos devs.

2 everyone: should I interpret lack of activity as lack of interest in getting this into Phobos or lack of issues to comment on? ;)

On 08/12/2013 03:27 PM, Dicebot wrote:
> Stepping up to act as a Review Manager for Jacob Carlborg std.serialization
>
> ---- Input ----
>
> Code: https://github.com/jacob-carlborg/phobos/tree/serialization
>
> Documentation:
> https://dl.dropboxusercontent.com/u/18386187/docs/std.serialization/index.html
>

I had no look at the code, but just opened the documentation, asking the question: "What do I need to do to serialize this graph data structure, I have here?". The documentation does not seem to give a straight answer.

Now, that's an issue I have with almost all phobos modules, for example with std.container, but I'll raise this point here: Our documentational standards are not good enough, because all we ever have is some API documentation ala this is module X, it contains the symbols A, B, C, which have a short description respectively.

However a good documentation (look at docs.python.org for example) needs to do more. The module has a purpose, because it should help me to accomplish a task. The documentation must say (preferably in a single location) what this task is and how this module/library may
help me in accomplishing it's task. An outline of the basic design decisions (for example where does std.container mention it's structures
have reference semantics?) are often invaluable in unterstanding api/
more detailed documentation.

I know how much work such documentation is and I surely wouldn't vote against std.serialization just because of this. But it's one of the two biggest hindrances if you want to get started / productive with D. (IMHO)

13-Aug-2013 16:48, Dicebot пишет:
> On Monday, 12 August 2013 at 19:55:01 UTC, Jacob Carlborg wrote:
>> I don't think a pull request should be made before a module has gone
>> through the review queue and is approved. With Github it's easy to
>> diff between a fork and upstream:
>> https://github.com/jacob-carlborg/phobos/compare/serialization
>
> Agreed.
>
>> * I forgot to add that the unit tests are, a bit controversial,
>> located in std.serialization.tests
>
> That is something that requires the input from Phobos devs.

IMHO a good idea to have a non-trivial test suite to be separate so that it doesn't not clutter other module(s). That said isolated tests for individual pieces and internal stuff are better kept together with the code they test.

-- 
Dmitry Olshansky

13-Aug-2013 17:15, Dicebot пишет:
> 2 everyone: should I interpret lack of activity as lack of interest in
> getting this into Phobos or lack of issues to comment on? ;)

Give us some time darn it ;)
-- 
Dmitry Olshansky

On 2013-08-13 16:34, Dmitry Olshansky wrote:

> IMHO a good idea to have a non-trivial test suite to be separate so that
> it doesn't not clutter other module(s). That said isolated tests for
> individual pieces and internal stuff are better kept together with the
> code they test.

They are of a high level nature and not for the internal stuff.

-- 
/Jacob Carlborg

On 2013-08-13 15:33, Tobias Pankrath wrote:

> I had no look at the code, but just opened the documentation, asking the
> question: "What do I need to do to serialize this graph data structure,
> I have here?". The documentation does not seem to give a straight answer.

There's a fully working example here:

https://dl.dropboxusercontent.com/u/18386187/docs/std.serialization/std_serialization_serializer.html

I worked quite hard with the documentation. There are code examples here as well, I just don't know where to put them in Phobos:

https://github.com/jacob-carlborg/orange/wiki/_pages

-- 
/Jacob Carlborg

On Tuesday, 13 August 2013 at 14:35:06 UTC, Dmitry Olshansky wrote:
> IMHO a good idea to have a non-trivial test suite to be separate so that it doesn't not clutter other module(s). That said isolated tests for individual pieces and internal stuff are better kept together with the code they test.

What do you think about having top-level folder with functional tests for a more complex packages?