Jump to page: 1 24  
Page
Thread overview
Formal Review of std.serialization
Jun 10, 2013
Jesse Phillips
Jun 10, 2013
Jonas Drewsen
Formal Review Process
Jun 10, 2013
Jesse Phillips
Jun 11, 2013
Jonathan M Davis
Jun 11, 2013
Jesse Phillips
Jun 11, 2013
Jesse Phillips
Jun 11, 2013
Jonathan M Davis
Jun 11, 2013
Jesse Phillips
Jun 11, 2013
Jonathan M Davis
Jun 12, 2013
Jesse Phillips
Jun 12, 2013
Jonathan M Davis
Jun 13, 2013
Jesse Phillips
Jun 13, 2013
Jonathan M Davis
Jun 13, 2013
Jacob Carlborg
Jun 13, 2013
SomeDude
Jun 13, 2013
Jesse Phillips
Jun 11, 2013
Jacob Carlborg
Jun 11, 2013
Jonas Drewsen
Jun 11, 2013
Dmitry Olshansky
Jun 11, 2013
Jonas Drewsen
Jun 11, 2013
Jacob Carlborg
Jun 11, 2013
Jacob Carlborg
Jun 11, 2013
Jesse Phillips
Jun 11, 2013
Jacob Carlborg
Jun 11, 2013
Jacob Carlborg
Jun 11, 2013
Jacob Carlborg
Jun 11, 2013
Jonas Drewsen
Jun 11, 2013
Jacob Carlborg
Jun 12, 2013
Dmitry Olshansky
Jun 13, 2013
Jacob Carlborg
Jun 15, 2013
Jacob Carlborg
Jun 15, 2013
Jacob Carlborg
June 10, 2013
Today we start the formal review of std.serialization (Orange). This library is authored by Jacob Carlborg and has been around through the D1/Tango days.

Please take some time in the next 2 weeks to provide feedback on the library. Some things to know (from http://forum.dlang.org/thread/kinpnv$ven$1@digitalmars.com)

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

* Trailing whitespace and tabs will be fixed when/if the package gets
accepted

And the author had some requests for specific things:

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

* 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. What are the options? These test are
quite high level. They test the whole Serializer class and not
individual functions.

* If this get accepted should I do a sub-tree merge (or what it's
called) to keep the history intact?

Source: https://github.com/jacob-carlborg/orange
Docs: https://dl.dropboxusercontent.com/u/18386187/orange_docs/Serializer.html

(For those not familiar with CandyDoc, There is a "Package" tab to view the tree of modules)
June 10, 2013
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.

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?

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.

util.traits.d : Most of them should go to std.traits or use something from std.traits instead if possible

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.

util.collection.array : should probably go into std.exception


It seems all the module filenames are camel cased. They should be all lowercase.

The _.d modules should probably be renamed to package.d now.


> * 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. What are the options? These test are
> quite high level. They test the whole Serializer class and not
> individual functions.

IMHO I think the tests would fit nicely into the package itself. Close to the code it tests.

> https://dl.dropboxusercontent.com/u/18386187/orange_docs/Serializer.html

Could you provide the docs formatted as it would be in dlang.org since only that way it is also possible to review formatting.

Keep up the good work. I for one are really looking forward to finally getting this thing in.

/Jonas


June 10, 2013
On Monday, 10 June 2013 at 21:40:58 UTC, 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.

While this is true and it would be my responsibility to help fit Orange into Phobos, there is no clear answer and is something community needs to give input into.

This project has already been through RFC and was not provided input during the "Ready for Review" announcement. So I have taken my self appointed position to push this into the formal platform were people will be required to voice failures they see for inclusion into Phobos.

Thank you for continuing the feedback, I will consider this particular state when deciding to call for a vote, remember that you can vote No and explain the hold up, if the state at voting is not something you feel can/should be included.
June 11, 2013
On Tuesday, June 11, 2013 01:06:07 Jesse Phillips wrote:
> On Monday, 10 June 2013 at 21:40:58 UTC, 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.
> 
> While this is true and it would be my responsibility to help fit Orange into Phobos, there is no clear answer and is something community needs to give input into.
> 
> This project has already been through RFC and was not provided input during the "Ready for Review" announcement. So I have taken my self appointed position to push this into the formal platform were people will be required to voice failures they see for inclusion into Phobos.
> 
> Thank you for continuing the feedback, I will consider this particular state when deciding to call for a vote, remember that you can vote No and explain the hold up, if the state at voting is not something you feel can/should be included.

I thought that it was clear that anything being submitted for review for inclusion in Phobos actually had to be in a state where a pull request for Phobos could be created for it. We certainly can't possibly vote it in if it's not in such a state, because we wouldn't even know what it would look like when it was merged in.

- Jonathan M Davis
June 11, 2013
On Tuesday, 11 June 2013 at 00:47:56 UTC, Jonathan M Davis wrote:
> I thought that it was clear that anything being submitted for review for inclusion in Phobos actually had to be in a state where a pull request for Phobos could be created for it.

There was no defining conclusion that I recall. There certainly were such opinions expressed, but as we don't have any review process for processes I don't see how a claim to clarity can exist for such matters.

I have provided my reason, the community is not providing the help contributers need during RFC.

> We certainly can't possibly vote it in if it's
> not in such a state, because we wouldn't even know what it would look like when it was merged in.
>
> - Jonathan M Davis

Sure you can, if the modifications are simple to understand then it is clear what it will be like in the long run. For those who feel the unknowns are too great, "No" is an appropriate vote. On the more important side, during review many times the issues are resolved prior to voting.

That said, we do need to figure out how to guide contributers when they hit known/unknown show stoppers. I've been lucky so far in working with libraries built specifically for Phobos with good quality, I've intended to take the things I've learned and put them into the wiki; which I shall state so I'll actually take responsibility to do so.

Thank you Jonathan, I will continue to think about how I can better help a contributer and suggestions are welcome.
June 11, 2013
On Mon, 10 Jun 2013 22:04:46 -0400, Jesse Phillips <Jesse.K.Phillips+D@gmail.com> wrote:

> On Tuesday, 11 June 2013 at 00:47:56 UTC, Jonathan M Davis wrote:
>> I thought that it was clear that anything being submitted for review for inclusion in Phobos actually had to be in a state where a pull request for Phobos could be created for it.
>
> There was no defining conclusion that I recall. There certainly were such opinions expressed, but as we don't have any review process for processes I don't see how a claim to clarity can exist for such matters.
>
> I have provided my reason, the community is not providing the help contributers need during RFC.

While I don't pretend to have been active at reviewing submissions, I have to agree with Jonathan.

Any code ready for review must have a clear indication of how the API will look when it's pulled into Phobos.  If it's not to that state, the code cannot really be reviewed as a possible contribution to Phobos.

I understand that there is some apprehension from Jacob on whether it's worth the effort of porting the code to be "phobos ready", but I can't see how anyone can possibly review the code based on the "merits" of the code.  The API is the most important part for code reviews!  We can fix implementation details later.

I have been in that position also, and it's not a fun place to be.

>> We certainly can't possibly vote it in if it's
>> not in such a state, because we wouldn't even know what it would look like when it was merged in.
>>
>> - Jonathan M Davis
>
> Sure you can, if the modifications are simple to understand then it is clear what it will be like in the long run. For those who feel the unknowns are too great, "No" is an appropriate vote. On the more important side, during review many times the issues are resolved prior to voting.

"Just imagine what the API would be like if it was in Phobos" isn't good enough.  Especially for this library, it looks like there are quite a few modules.  Where do those go?  What changes are made?  Standing out right away is

We need an API document of what it would be in Phobos, and how the API works.

I would recommend suspending this review, putting together a strawman API of how you think it will look under phobos, post that as an RFC, and then judge whether it's worth porting from that response.  If there's only a subset of the API we should be looking at that, show me that subset.

We simply cannot have a formal review on software that isn't ready for inclusion - by definition we would have to have another review later when it's ready for inclusion.

-Steve
June 11, 2013
On Tuesday, 11 June 2013 at 02:46:07 UTC, Steven Schveighoffer wrote:
> Any code ready for review must have a clear indication of how the API will look when it's pulled into Phobos.  If it's not to that state, the code cannot really be reviewed as a possible contribution to Phobos.

I have to stop you there, it appears your premise is off. This is not about the API being ready for Phobos, this is about having a pull request-able submission. As you say a little bit later the implementation details can be worked out later, or what I'm advocating, we can decided where his util parts fit into Phobos later.
June 11, 2013
On Tuesday, June 11, 2013 05:24:53 Jesse Phillips wrote:
> On Tuesday, 11 June 2013 at 02:46:07 UTC, Steven Schveighoffer
> 
> wrote:
> > Any code ready for review must have a clear indication of how the API will look when it's pulled into Phobos. If it's not to that state, the code cannot really be reviewed as a possible contribution to Phobos.
> 
> I have to stop you there, it appears your premise is off. This is not about the API being ready for Phobos, this is about having a pull request-able submission. As you say a little bit later the implementation details can be worked out later, or what I'm advocating, we can decided where his util parts fit into Phobos later.

The whole _point_ of an official review is to review the API that would end up in Phobos (the implementation is also important but very much secondary). If a submission's API isn't ready to be merged into Phobos assuming that it passed the vote, then it isn't ready for review. Period. I don't see how this could even be debatable. Sure, the end result may be quite a bit different from the original API depending on what happens in the review, but the review process is for ironing out the final kinks in the API, not for designing it.

Sure, prior to the API being ready, you can submit code for folks to review for you, but that's completely different from an official Phobos review. The whole point of that is to review and vote on the actual API that's going to end up in Phobos.

- Jonathan M Davis
June 11, 2013
On Mon, 10 Jun 2013 23:24:53 -0400, Jesse Phillips <Jesse.K.Phillips+D@gmail.com> wrote:

> On Tuesday, 11 June 2013 at 02:46:07 UTC, Steven Schveighoffer wrote:
>> Any code ready for review must have a clear indication of how the API will look when it's pulled into Phobos.  If it's not to that state, the code cannot really be reviewed as a possible contribution to Phobos.
>
> I have to stop you there, it appears your premise is off. This is not about the API being ready for Phobos, this is about having a pull request-able submission.

Having a pull requestable submission requires having an API ready for phobos.  How do you have step 2 without step 1?

> As you say a little bit later the implementation details can be worked out later, or what I'm advocating, we can decided where his util parts fit into Phobos later.

So where is the API?  The link you posted is not ready to be part of Phobos, and there is no clear indication of how it will reside in Phobos.

Please make that clear.

-Steve
June 11, 2013
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
« First   ‹ Prev
1 2 3 4