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

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

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

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

> 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

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?

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

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

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.

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.