www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Formal Review of std.serialization

reply "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
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)
Jun 10 2013
next sibling parent reply "Jonas Drewsen" <nospam4321 hotmail.com > writes:
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
Jun 10 2013
next sibling parent Jacob Carlborg <doob me.com> writes:
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
Jun 10 2013
prev sibling next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-06-11 02:47, 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. 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.

A quick answer: * 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 The rest I'm not sure about. I can put it in std.traits and other places where I see it fit. I'll start with merging it with Phobos. -- /Jacob Carlborg
Jun 10 2013
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 6/11/13 2:36 AM, Jacob Carlborg wrote:
 I'll start with merging it with Phobos.

If that's the case I suggest we suspend the voting process to be fair to the submitter (allow time to integrate feedback etc). Andrei
Jun 11 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-06-11 15:36, Jesse Phillips wrote:

 I have suspended the review.

 Jabob, please take all supporting libraries and hide them from the
 public API since if you guess wrong the review well be even less likely
 to be about a serialization library.

I'll do that. -- /Jacob Carlborg
Jun 11 2013
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-06-11 17:13, Steven Schveighoffer wrote:

 OK, this is good, clearer.

 This should be std.serialization.archives.xmlarchive?

Yes.
 It would be good to know what is being merged in, and where.  Or as I
 read your response elsewhere, you will make them private for now, good.

 I'll start with merging it with Phobos.

I will take a look at the existing API in the meantime. Knowing how it will fit in phobos makes this easier, thanks.

Thanks. I have started to merge with Phobos now. I've got a basic "hello world" working. This code works now, after replacing imports. https://github.com/jacob-carlborg/orange#simple-usage-example I will now being adding unit tests and fix compile errors as I hit them. See: https://github.com/jacob-carlborg/phobos/tree/serialization Note, the merge is not complete yet, but I guess no API changes are necessary. -- /Jacob Carlborg
Jun 11 2013
prev sibling next sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-06-11 04:46, Steven Schveighoffer wrote:

 "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.

The API is ready, see my post to Jonathan. http://forum.dlang.org/thread/adyanbsdsxsfdpvoozne forum.dlang.org?page=2#post-kp6gi4:242tvc:241:40digitalmars.com -- /Jacob Carlborg
Jun 10 2013
prev sibling next sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-06-11 05:48, Steven Schveighoffer wrote:

 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.

See my previous post: http://forum.dlang.org/thread/adyanbsdsxsfdpvoozne forum.dlang.org?page=2#post-kp6gi4:242tvc:241:40digitalmars.com The utility functions are less important. -- /Jacob Carlborg
Jun 10 2013
prev sibling parent Dmitry Olshansky <dmitry.olsh gmail.com> writes:
11-Jun-2013 13:00, Jonas Drewsen пишет:
 I really think that all the general stuff in the utils/core etc.
 packages should be merged into phobos as the very first thing and as
 normal pull requests to phobos. They are not serialization specific and
 rather small additions.

 After that the std.serialization part could go through normal review and
 would simpler to review.

 /Jonas

Rather what we usually do with helpers of unknown utility etc. is to keep them package/private for now and let their fate be decided separately via normal pull request process. The fact that we have no good idea where to put (if at all) an internal primitive should not (I'd say must not) postpone or undermine the review of the module/package itself. -- Dmitry Olshansky
Jun 11 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
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.
Jun 10 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
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
Jun 10 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
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.
Jun 10 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
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
Jun 10 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
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.
Jun 10 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
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
Jun 10 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
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
Jun 10 2013
prev sibling next sibling parent "Jonas Drewsen" <nospam4321 hotmail.com > writes:
 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.

Actually I was thinking about just moving the test dir into the std.serialization. It shouldn't be part of the final distribution of course so the Makefiles would have to take that into consideration. /Jonas
Jun 11 2013
prev sibling next sibling parent "Jonas Drewsen" <nospam4321 hotmail.com > writes:
I really think that all the general stuff in the utils/core etc. 
packages should be merged into phobos as the very first thing and 
as normal pull requests to phobos. They are not serialization 
specific and rather small additions.

After that the std.serialization part could go through normal 
review and would simpler to review.

/Jonas
Jun 11 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
On Tuesday, 11 June 2013 at 12:22:28 UTC, Andrei Alexandrescu 
wrote:
 On 6/11/13 2:36 AM, Jacob Carlborg wrote:
 I'll start with merging it with Phobos.

If that's the case I suggest we suspend the voting process to be fair to the submitter (allow time to integrate feedback etc). Andrei

I have suspended the review. Jabob, please take all supporting libraries and hide them from the public API since if you guess wrong the review well be even less likely to be about a serialization library.
Jun 11 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
On Tuesday, 11 June 2013 at 03:36:23 UTC, Jonathan M Davis wrote:
 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).

Then what are you complaining about?
 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.

The whole point of an official review is to decided if the API is ready to be merged into Phobos. A review manager can't make that decision, he brings it to the community and has them decided, "Is this API what we would like to see for handling ____?" and the community votes yes or no. Phobos is lacking in functionality to support Jabob's submission. I think it is wrong to require that Phobos be fixed prior to a formal review. Since Steven brought up the API your taking issue with that, but I'm wondering about the Phobos maintainers views on the implementation details not being ready for Phobos. I don't think an addition passing votes means that it must be included into Phobos right away (we don't have std.uni due to a failed test on BSD). As long as there is agreement on direction, for example it should use std.xml when we actually have a replacement for it, then things should be fine. And if there are many changes needed then a vote can be postponed until after the changes are complete and a review wouldn't be needed again unless the API changed.
Jun 11 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Tue, 11 Jun 2013 02:36:51 -0400, Jacob Carlborg <doob me.com> wrote:

 On 2013-06-11 02:47, 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. 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.

A quick answer:

OK, this is good, clearer.
 * orange.serialization.archives.XmlArchive ->  
 orange.serialization.archives.xmlarchive

This should be std.serialization.archives.xmlarchive?
 The rest I'm not sure about. I can put it in std.traits and other places  
 where I see it fit.

It would be good to know what is being merged in, and where. Or as I read your response elsewhere, you will make them private for now, good.
 I'll start with merging it with Phobos.

I will take a look at the existing API in the meantime. Knowing how it will fit in phobos makes this easier, thanks. -Steve
Jun 11 2013
prev sibling next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Tuesday, June 11, 2013 15:55:30 Jesse Phillips wrote:
 On Tuesday, 11 June 2013 at 03:36:23 UTC, Jonathan M Davis wrote:
 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).

Then what are you complaining about?

The way this submission is currently laid out, it couldn't possibly be merged into Phobos. If you can't create a pull reuest from what you have, then the API isn't ready. It needs to be laid out in a manner that we can see what the actual API would be if it were merged into Phobos. In this case, Jacob's submission is being presented as a 3rd party library rather than as how it would look as part of Phobos. If it's really ready for possible inclusion in Phobos, then it shouldn't be hard to create a version of it as a pull request for Phobos and present that, in which case we could see what its actual API in Phobos would be. If that can't be done, then I don't see how it could be ready for review. No matter how cool a 3rd party library may be, we're not reviewing 3rd party libraries. We're reviewing new additions to Phobos, and submissions should be presented as such.
 Phobos is lacking in functionality to support Jabob's submission.
 I think it is wrong to require that Phobos be fixed prior to a
 formal review.

As Dmitry points out, some of that stuff can be kept internal for the time being and then separated out and move to the proper place in Phobos later. I believe that some of that happened with internals of std.regex and what's now going to be in std.uni. - Jonathan M Davis
Jun 11 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-06-13 05:42, Jesse Phillips wrote:

 This has been what I'm challenging. The problem I'm trying to solve, and
 probably inappropriately doing so as part of the formal review, is as
 below.

 1. We do not have a formal way to confirm a library should be put into
 Phobos
 2. When Phobos does not provide the supporting modules we do not have a
 formal way to answer "Where do they fit in?"

How about we just stop the reviewing of std.serialization right now. This arguing is leading nowhere. We can push back std.serialization one place in the review queue and start with the next module in line. We can continue reviewing std.serialization the next time it's time for a new module to be reviewed. By then I will have finished merging it into Phobos. -- /Jacob Carlborg
Jun 12 2013
prev sibling next sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-06-10 18:11, Jesse Phillips wrote:
 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)

I have started to merge with Phobos now. I've got a basic "hello world" working. This code works now, after replacing imports. https://github.com/jacob-carlborg/orange#simple-usage-example I will now being adding unit tests and fix compile errors as I hit them. See: https://github.com/jacob-carlborg/phobos/tree/serialization Note, the merge is not complete yet, but I guess no API changes are necessary. -- /Jacob Carlborg
Jun 11 2013
prev sibling next sibling parent "Jonas Drewsen" <nospam4321 hotmail.com > writes:
 Rather what we usually do with helpers of unknown utility etc. 
 is to keep them package/private for now and let their fate be 
 decided separately via normal pull request process.

That is an ok way to go as well. Most of the helpers are not of unknown utility though.
 The fact that we have no good idea where to put (if at all) an 
 internal primitive should not (I'd say must not) postpone or 
 undermine the review of the module/package itself.

I think many of the helpers fit pretty well in some of the existing packages and it would feel more streamlined if they were moved there. I'm simply suggesting a divide and conquer technique for getting orange into phobos with the benefit of reduce review scope in the end. Getting RFC comments on orange has not been very fruitful so far and maybe reducing scope could do the trick. Just an idea. /Jonas
Jun 11 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
Please, don't get caught up in my failure to provide a clean 
representation of the API, the more reasoning for how we are 
doing things the better the documentation.

On Tuesday, 11 June 2013 at 17:33:59 UTC, Jonathan M Davis wrote:
 If you can't create a pull reuest from what you  have,
 then the API isn't ready.

I am already aware this is your opinion. I'm looking for the correlation of being able create a pull request and an API being ready.
 It needs to be laid out in a manner that we can see what the 
 actual API would be if it were merged into Phobos.

Exactly as it is present, the only problem I see is that several helper packages exist and their documentation is present. Look again at the package list: https://dl.dropboxusercontent.com/u/18386187/orange_docs/Serializer.html Now collapse core, util, and xml. Please point out to me, what of that does not look ready for inclusion into Phobos? Is it because "orange" is the top package and not std? Is it because there are 7 modules and 2 packages? Is it because the docs were generated with CandyDoc? I realize at this point that having documentation for the supporting libraries was a mistake, I'm now looking for concrete examples of why this would have been an issue if they had been hidden to begin with, or is this purely a principled stance? Another piece of information I'm interested in, is your adamant request for a pull request-able code for the internal code review. Meaning as a Phobos maintainer you can see how the supporting libraries are stored? Or are you concerned that the public API will need to change for a 3rd party library to fit into Phobos?
Jun 11 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, June 12, 2013 05:04:41 Jesse Phillips wrote:
 Another piece of information I'm interested in, is your adamant
 request for a pull request-able code for the internal code
 review. Meaning as a Phobos maintainer you can see how the
 supporting libraries are stored? Or are you concerned that the
 public API will need to change for a 3rd party library to fit
 into Phobos?

If it's not pull-request ready, then it's not showing how it would fit into Phobos. What we have here is a link to a 3rd party project laid out as its own project complete with the full built setup. Which pieces are supposed to go into Phobos? All of them? Just some of them? Where do they fit into Phobos? How will moving things around enough to make it ready for a pull request affect the package layout and API? By having a submission that could be merged into Phobos as-is, it's clear what the API is, what is really being submitted for review, and how it'll fit into Phobos. And since the submission has to be turned into a pull request in order to be merged in anyway, why not just do it up front? Without that, it's not clear what's actually being submitted. And it doesn't say good things about the submitter and their willingness to get the code ready if they're not willing to put their submission in a pullable state in order to get it reviewed. I don't know if you gave Jacob a chance to do that before officially starting the review or not (and by the sound of it, he _is_ now getting it pull request ready), but he's already shown quite a bit of resistance in the past to adjusting his code to match Phobos, so it just plain looks bad when the submission isn't in a state where it could be merged into Phobos if it were deemed to be good enough for it, regardless of what the actual intentions of the submitter are. It sounds like the wiki probably needs clearer guidelines as to what is required of submissions to the review queue, but I don't see how anyone could think that something was ready for review when it isn't even in a state where it shows what it would look like were it to be merged into Phobos. - Jonathan M Davis
Jun 11 2013
prev sibling next sibling parent Dmitry Olshansky <dmitry.olsh gmail.com> writes:
10-Jun-2013 20:11, Jesse Phillips пишет:
 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)

If/when I have time next week I'll try to integrate EBML serialization with orange as a custom archive, if it works out fine I'm all for this module :)
 * The most important packages are: orange.serialization and
 orange.serialization.archives

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

No harm just fixing those right away - in any case you have git and can rollback anything. -- Dmitry Olshansky
Jun 12 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
On Wednesday, 12 June 2013 at 03:23:54 UTC, Jonathan M Davis 
wrote:
 If it's not pull-request ready, then it's not showing how it 
 would fit into Phobos.

I agree.
 Where do they fit into Phobos?

This has been what I'm challenging. The problem I'm trying to solve, and probably inappropriately doing so as part of the formal review, is as below. 1. We do not have a formal way to confirm a library should be put into Phobos 2. When Phobos does not provide the supporting modules we do not have a formal way to answer "Where do they fit in?" I'm having a hard time concisely expressing myself with this. In principle I completely understand why you're taking this position, but practically I don't see it. Let me go back to real examples: https://github.com/jacob-carlborg/orange Orange has a directory for 'tests.' Phobos has generally not had unittests separate from the function/module being tested. Is that unacceptable? I don't know, there certainly was no objection when asked. The repository provides a means to build the source into a library. Does it really make it harder to review because someone can build and use the code without checking out dmd, druntime, phobos and building themselves a testable Phobos library? There is also a wiki folder, it has an orange ball. Ok, yes there are distractions. I'll admit, I started from the docs, diving into the code as I needed and had I started from the github repository it wouldn't have been obvious where things would go in Phobos.
Jun 12 2013
prev sibling next sibling parent "SomeDude" <lovelydear mailmetrash.com> writes:
On Tuesday, 11 June 2013 at 13:55:31 UTC, Jesse Phillips wrote:
 On Tuesday, 11 June 2013 at 03:36:23 UTC, Jonathan M Davis 
 wrote:
 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).

Then what are you complaining about?
 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.

The whole point of an official review is to decided if the API is ready to be merged into Phobos. A review manager can't make that decision, he brings it to the community and has them decided, "Is this API what we would like to see for handling ____?" and the community votes yes or no.

*accepted* into Phobos. That is to say, it's already merged and mostly debugged. You can call your process whatever you want, but it's *not* a "formal review process". It's more like a RFC.
Jun 12 2013
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, June 13, 2013 05:42:43 Jesse Phillips wrote:
 On Wednesday, 12 June 2013 at 03:23:54 UTC, Jonathan M Davis
 
 wrote:
 If it's not pull-request ready, then it's not showing how it
 would fit into Phobos.

I agree.
 Where do they fit into Phobos?

This has been what I'm challenging. The problem I'm trying to solve, and probably inappropriately doing so as part of the formal review, is as below. 1. We do not have a formal way to confirm a library should be put into Phobos

What's there to confirm? Anyone who wants it to be in Phobos can prepare it for review, and if it passes the vote, it's in. If the programmer doing it wants to try and figure out ahead of time whether a particular library or piece of functionality might be acceptable, they can just ask, and while that may not give a definitive answer, it should avoid the cases where it's an obvious no.
 2. When Phobos does not provide the supporting modules we do not
 have a formal way to answer "Where do they fit in?"

I would expect that the answer to that is generally either: 1. Get the supporting stuff into Phobos first. or 2. Make it private to what's being submitted for review, and the private functionality can be moved into the public API where appropriate later (either through pull requests if it's small or other reviews if it's large - though if it's particularly large, I would think that it would be best to get it into Phobos on its own first). It's already happened in the past that some functionality has been private to a module and not been put into Phobos' public API. That's essentially what happened with portions of std.regex which have recently been voted in as part of the new std.uni.
 I'm having a hard time concisely expressing myself with this. In
 principle I completely understand why you're taking this
 position, but practically I don't see it.
 
 Let me go back to real examples:
 https://github.com/jacob-carlborg/orange
 
 Orange has a directory for 'tests.' Phobos has generally not had
 unittests separate from the function/module being tested. Is that
 unacceptable? I don't know, there certainly was no objection when
 asked.

I believe that I've objected every time that it's been suggested and I was involved in the thread, but I don't know what the consensus of the Phobos devs would be on that.
 The repository provides a means to build the source into a
 library. Does it really make it harder to review because someone
 can build and use the code without checking out dmd, druntime,
 phobos and building themselves a testable Phobos library?
 
 There is also a wiki folder, it has an orange ball. Ok, yes there
 are distractions.
 
 I'll admit, I started from the docs, diving into the code as I
 needed and had I started from the github repository it wouldn't
 have been obvious where things would go in Phobos.

I think that it pretty much comes down to this: Someone who wants to submit something to Phobos gets it to the point that it could be merged into Phobos as-is if it were voted in. If there are questions, they can ask in the newsgroup ahead of time, but in some cases, they're likely just going to have to decide themselves on what they think the best way to do it is. If such a decision is not acceptable, then it can then be examined as part of the review process. But essentially what it comes down to is that the submitter submits something which they think is in a state which could be merged into Phobos, and then it's examined during the review process, and the feedback generated during the review process leads to whatever adjustments need to be made. And if it still isn't acceptable at that point, then presumably, it'll fail the vote (or not even get voted on if it's clearly not ready yet). - Jonathan M Davis
Jun 12 2013
prev sibling next sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-06-10 18:11, Jesse Phillips wrote:
 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)

I have finished merging std.serialization into Phobos. Branch is available here: https://github.com/jacob-carlborg/phobos/tree/serialization I haven't been able to build the documentation yet, I'm getting this error: $ make -f posix.mak html DOC_OUTPUT_DIR=/Users/jacob/development/d/dlang/d-programming-language.org/web/phobos-prerelease make: *** No rule to make target `/Users/jacob/development/d/dlang/d-programming-language.org/web/phobos-prerelease/std_serializat on_attribute.html', needed by `html'. Stop. It's up to you if we should continue with the review at this point. -- /Jacob Carlborg
Jun 13 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
On Thursday, 13 June 2013 at 06:00:15 UTC, SomeDude wrote:
 Nope. The whole point of a review process is to see if it can 
 be *accepted* into Phobos. That is to say, it's already merged 
 and mostly debugged.

In that case, we need to make sure the formal review includes a Phobos dev that has examined the integration into Phobos and has made approval for this specific requirement.
Jun 13 2013
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-06-10 18:11, Jesse Phillips wrote:
 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)

I have finally been able to generate the documentation with the proper styles: file:///Users/doob/Dropbox/Public/docs/std.serialization/index.html -- /Jacob Carlborg
Jun 15 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-06-15 13:53, Jacob Carlborg wrote:

 file:///Users/doob/Dropbox/Public/docs/std.serialization/index.html

Of course I copied the wrong link :( Here's the correct one: https://dl.dropboxusercontent.com/u/18386187/docs/std.serialization/index.html -- /Jacob Carlborg
Jun 15 2013