On 3/17/14, 12:28 PM, Jacob Carlborg wrote:
> On 2014-03-17 20:07, Walter Bright wrote:
>
>> ScopeBuffer has been there and commented on for about 2 months now.
>
> That's because you created a pull request instead of asking for a formal
> review. A pull request should NOT be created until a module is accepted
> after a review and voting.
>
> You're sneaking in a new module although it has not gone through a
> formal review. This is not how new modules should be added.

I had the same concerns about it being front and center in std. Now that it's internal that issue disappears - we can use it inside Phobos for a while and change it without disrupting users. In many ways putting it up for review after all makes things better for everybody.

Andrei

Apologies if this was answered elsewhere, but will this, or something similar, be exposed in Phobos for others to use?  This seems like something that could be quite useful depending on the application.  If not, why?

On Monday, 17 March 2014 at 20:51:05 UTC, sybrandy wrote:
> Apologies if this was answered elsewhere, but will this, or something similar, be exposed in Phobos for others to use?  This seems like something that could be quite useful depending on the application.  If not, why?

FWIW, I've also begun working on something similar which (I think) would be more acceptable for "public" inclusion. In particular, that would provide better memory safety (subset usable in @safe code), CTFE-able, and pure. And faster than Appender.

On Monday, 17 March 2014 at 20:20:46 UTC, Walter Bright wrote:

> Internal modules do not need the formal review process, since they are not documented and not part of the public facing API, any more than other changes to the internals of Phobos functions need such review.

That's how it ended up being merged. That's not how it started. You created a pull request for a public visible new module without asking for a formal review. You have done this before. Please stop doing this.

> The review of ScopeBuffer on the github PR threads has been far more thorough than about any other. It's up to 260 comments on about 50 lines of actual code over about 2 months.

Doesn't matter. Just follow the protocols. What's the problem with that?

--
/Jacob Carlborg

On Monday, 17 March 2014 at 20:31:36 UTC, Andrei Alexandrescu wrote:

> I had the same concerns about it being front and center in std. Now that it's internal that issue disappears - we can use it inside Phobos for a while and change it without disrupting users. In many ways putting it up for review after all makes things better for everybody.

I don't think that issue disappears. Walter has done this before and now he did it again. Creating a pull request for a new module without asking for a formal review. It's less of a problem that it was merged as an internal module. The big issue trying to get everyone to follow the protocols we have for adding new modules.

--
/Jacob Carlborg

On Tuesday, 18 March 2014 at 07:45:03 UTC, Jacob Carlborg wrote:
> On Monday, 17 March 2014 at 20:31:36 UTC, Andrei Alexandrescu wrote:
>
>> I had the same concerns about it being front and center in std. Now that it's internal that issue disappears - we can use it inside Phobos for a while and change it without disrupting users. In many ways putting it up for review after all makes things better for everybody.
>
> I don't think that issue disappears. Walter has done this before and now he did it again. Creating a pull request for a new module without asking for a formal review. It's less of a problem that it was merged as an internal module. The big issue trying to get everyone to follow the protocols we have for adding new modules.

In his defense, he *is* trying to keep our modules clean, which implies creating a new module. But let's put this into perspective:

Said module is *1* type only. It's not like it was a new 10_000 line module or anything: It's 100 lines + unittest.

If it was me, I would have just put the damn thing as a new type in "std.array", submitted it as a pull, with request for review nor mention in the boards, and then be done with it. And *then*, if we weren't happy about it, I would have marked it package too.

It's not the first time we do this too. There's a fair bit in things in phobos that are "fairly useful, but don't justify public exposure". This is just one more of them. It just happens to be in a module.

I'm not entirely happy about the way it went down, but I think we should cut him a little slack in that regards.

That, and I think there was bad communication. On both sides.

On Monday, 17 March 2014 at 19:07:44 UTC, Walter Bright wrote:
> On 3/17/2014 11:10 AM, Dicebot wrote:
>> 1)
>> Walter has been pushing for getting this through the review queue to the point
>> where I needed to ask Brian to delay voting for his module and switch to
>> proceeding with Walter's. It didn't do any harm this time as Brian got busy
>> anyway but I am very unhappy that I even had to do it.
>>
>> Now it suddenly gets cancelled and merged, internal or not (the very existence
>> of std.internal rings a bell but it is a different story). Why bother me and
>> push on Brian if you are just going to hurry merge it?
>
> The ongoing threads about it made it clear that it was never going to happen as std.buffer.scopebuffer. I had assumed you were monitoring that, and I shouldn't have assumed so. I apologize.

You see, review process exists for the very reason this is not clear even if it seems so. I will never take the courage of judging early termination of review simply because it does not seem to succeed. If anything, I'd try to encourage as much different input as possible to make decision that is clear to external observer.

If you want something internal, you just go for it. If you want something public and reviewed, changing your mind few days after review request is not something that leaves a good impression. Consider how an outsider that does not read NG daily may see it.

We always could have added something needed immediately as internal module _AND_ proceeded with review of possible higher level alternative than can fit public Phobos.

> The objective technical issues raised were all addressed, and the immutable/const one was corrected and unittests added for it before it was pulled.

Ok, I have probably not noticed that between the noise which leads us to...

> Some of the issues were subjective, where there are no clearly right or wrong answers, and a decision needs to be made at some point.

> ScopeBuffer has been there and commented on for about 2 months now. At last count it had over 4 comments per line of code. It is probably the most reviewed PR ever.

..it is not something to be proud of. It has got that many comments because you started to argue against style comments and queries for some performance data. That is completely out of the line of normal PR review. It does not matter if you judgement is right or wrong here - such approach simply creates too much noise over things that are not truly important.

This is also the reason why high-level review happens before creating the pull - hard to stay focused otherwise.

> It is necessary to resolve a serious issue we have with Phobos that comes up constantly in public discussions about D. At some point we've got to move on and resolve this.

I was among those who has been continuously asking for cleaning Phobos allocations and I feel  that this modules about zero of issues I see. API allocations are much more important to fix than internal allocations and you still have not answered why you consider scopebuffer of more priority.

Also while it is important it is not at any hurry and shouldn't be done hastily simply because it is next discussion topic of the month.

On 3/18/14, 12:45 AM, Jacob Carlborg wrote:
> On Monday, 17 March 2014 at 20:31:36 UTC, Andrei Alexandrescu wrote:
>
>> I had the same concerns about it being front and center in std. Now
>> that it's internal that issue disappears - we can use it inside Phobos
>> for a while and change it without disrupting users. In many ways
>> putting it up for review after all makes things better for everybody.
>
> I don't think that issue disappears. Walter has done this before and now
> he did it again. Creating a pull request for a new module without asking
> for a formal review. It's less of a problem that it was merged as an
> internal module. The big issue trying to get everyone to follow the
> protocols we have for adding new modules.

I don't think we need a formal review process for internal modules. Proper review of the pull request should suffice.

Andrei

On 2014-03-18 16:59, Andrei Alexandrescu wrote:

> I don't think we need a formal review process for internal modules.
> Proper review of the pull request should suffice.

I agree. But this module _started out_ as a public module. If we have had a formal review we could have discussed and decided in the review that the module doesn't fit to be public.

-- 
/Jacob Carlborg

On 3/18/14, 1:53 PM, Jacob Carlborg wrote:
> On 2014-03-18 16:59, Andrei Alexandrescu wrote:
>
>> I don't think we need a formal review process for internal modules.
>> Proper review of the pull request should suffice.
>
> I agree. But this module _started out_ as a public module. If we have
> had a formal review we could have discussed and decided in the review
> that the module doesn't fit to be public.

Instead we decided much quicker so it's all for the better. I really don't understand why the bickering about this still continues.

Andrei