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 reply "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
parent reply "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
next sibling parent reply "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
parent reply "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
next sibling parent reply "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
next sibling parent reply "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
parent reply "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
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 reply "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
parent reply "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
parent reply "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
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 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 parent reply "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.


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.

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
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 "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
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 reply "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
parent reply 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
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 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 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 reply "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
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 reply "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
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 parent reply 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
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 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 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 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 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