On Sunday, 9 March 2014 at 17:25:31 UTC, Andrei Alexandrescu wrote:
> Apparently direct use of the untagged union elements was not being unittested.

Were they documented? How did their use end up in production code?

On 3/9/14, 6:49 AM, Andrej Mitrovic wrote:
> On 3/9/14, Walter Bright <newshound2@digitalmars.com> wrote:
>> BTW, I don't know if better unittest coverage would have detected this
>> particular breakage, but in any case, we should do better with coverage.
>
> Can someone finally say *what* broke?

I was wrong in my previous message. As Adam noted, it was making type read-only. Previously people would write:

v.type = JSON_TYPE.STRING;
v.str = "abc";

Now the second line sets the type, and the first is an error.

Also, this code is in error:

v.type = JSON_TYPE.TRUE;

and must be replaced with:

v = true;


Andrei

On Sunday, 9 March 2014 at 17:30:32 UTC, Andrei Alexandrescu wrote:
> v.type = JSON_TYPE.TRUE;
>
> and must be replaced with:
>
> v = true;
>
>
> Andrei

I see why you'd want to make this kind of change. It was probably a mistake to let you change the tag like that, what could be done to fix this is this.

deprecated("Don't assign to type!") @property void type(JSON_TYPE new_type)
{
    type_tag = new_type;
}

Then the code would probably work like before and people using x.type = ... in production would start seeing a deprecation warning instead, which we could make into an error at a later date.

On Sunday, 9 March 2014 at 17:30:32 UTC, Andrei Alexandrescu wrote:
> On 3/9/14, 6:49 AM, Andrej Mitrovic wrote:
>> On 3/9/14, Walter Bright <newshound2@digitalmars.com> wrote:
>>> BTW, I don't know if better unittest coverage would have detected this
>>> particular breakage, but in any case, we should do better with coverage.
>>
>> Can someone finally say *what* broke?
>
> I was wrong in my previous message. As Adam noted, it was making type read-only. Previously people would write:
>
> v.type = JSON_TYPE.STRING;
> v.str = "abc";
>
> Now the second line sets the type, and the first is an error.
>
> Also, this code is in error:
>
> v.type = JSON_TYPE.TRUE;
>
> and must be replaced with:
>
> v = true;

I think this is something that can be easily fixed without causing more regressions. Have you filed it as an issue yet?

On 3/9/14, 10:43 AM, Vladimir Panteleev wrote:
> I think this is something that can be easily fixed without causing more
> regressions. Have you filed it as an issue yet?

https://d.puremagic.com/issues/show_bug.cgi?id=12332

Andrei

On Sunday, 9 March 2014 at 17:51:54 UTC, Andrei Alexandrescu wrote:
> On 3/9/14, 10:43 AM, Vladimir Panteleev wrote:
>> I think this is something that can be easily fixed without causing more
>> regressions. Have you filed it as an issue yet?
>
> https://d.puremagic.com/issues/show_bug.cgi?id=12332

https://github.com/D-Programming-Language/phobos/pull/1988

On 3/9/2014 11:18 AM, Vladimir Panteleev wrote:
> https://github.com/D-Programming-Language/phobos/pull/1988

Thanks for the quick action, Vladimir!

On Sunday, 9 March 2014 at 03:58:31 UTC, Andrei Alexandrescu wrote:
> std.json broke backward compatibility, starting with https://github.com/D-Programming-Language/phobos/commit/1958c95666b0241d669d282806e4f724fbb37caf. There's even no notice in the changelog of the breakage, which is quite obvious by inspecting the diff.
>
> This is a serious matter because it broke production code. Was there a reason for the breakage? We should make sure breaking changes are avoided or get a lot of scrutiny if they are really necessary.
>
> I wonder how we can improve the process to avoid such issues in the future.
>
>
> Andrei

While this breakage was gratuitous, I would have expected std.json to have this infamous warning by now:

"Warning: This module is considered out-dated and not up to Phobos' current standards. It will remain until we have a suitable replacement, but be aware that it will not remain long term."

It's a pretty crap module and it's a fact that we are looking for a replacement. Maybe this caused everyone involved to lower their standard for proper unit tests and general scrutiny.

On 3/13/14, Jakob Ovrum <jakobovrum@gmail.com> wrote:
> It's a pretty crap module and it's a fact that we are looking for a replacement.

It clearly didn't have the proper initial unittests for the API which broke. The pull request which introduced the regression did not modify existing unittests, it only added new ones. The pull went through a review just like any other pull request, there's nobody going "oh this updates std.xml, I'll just merge this regardless because std.xml sucks".