Fix Plotly/PlotlyJS bounds

[TotalVerb]

• Plotly does not require any particular version of JSON
• PlotlyJS versions before 0.3.0 require at most 0.5.1. Changes introduced in 0.5.2 and kept in 0.5.3 broke PlotlyJS.
• PlotlyJS versions 0.3.0 or later require at least 0.5.2. I set it to 0.5.3 to avoid downgrade/upgrade surprises, since 0.5.2 is the same as 0.6.

[tkelman]

Changes introduced in 0.5.2 and kept in 0.5.3

that sounds like I didn't check 0.5.3 carefully enough, @kmsquire I thought the intent was for 0.5.1 and 0.5.3 to be the same - what does 0.5.3 have that 0.5.1 doesn't?

[TotalVerb]

The change that broke Plotly was that the overloaded Base.colon methods to JSON printer state was removed. I'm not familiar enough with PlotlyJS to know why it depended on those methods.

[tkelman]

I dislike using a nonzero patch version in an upper bound since that prevents fixing other bugs within the same minor series. We could alternately make an 0.5.4 that actually is the same as 0.5.1, what did removing the colon methods accomplish?

[TotalVerb]

Making a 0.5.4=0.5.3 before this PR is merged will break PlotlyJS again. The package is depending on JSON.jl's internal implementation details for some reason.

Removing the colon methods was just something I noticed when looking at methods of Base.colon and finding two unrelated methods for JSON printing purposes. It was a method pun.

[tkelman]

That sounds like the underlying issue and just asking for trouble. @ssfrr @spencerlyon2 (sorry wrong spencer) can PlotlyJS please use only the public API?

[TotalVerb]

I think JSON.separator can be made into public API if there's a demand. The underlying issue is that people are overloading JSON._print. This is not very kosher because _print is supposed to be internal, from my understanding, but it's very popular. This is probably because there's not really a good way to define custom JSON serialization currently. I'll file an issue for that on JSON.jl's side. Edit: JuliaIO/JSON.jl#150

JSON._print calls separator (née Base.colon) to determine the correct (one of ": " and ":") separator character for printing purposes. PlotlyJS is using what amounts to a duplicate of JSON's code to serialize its own kind object as JSON, which strikes me as a pretty bad API.

[tkelman]

What's very strange is PlotlyJS never had its tests fail, which is a sign that it isn't testing features that Plotly is using. If PlotlyJS is going to use the internals of JSON in that messy of a way, then it should really use an exact upper and lower bound.

[TotalVerb]

How's this solution? I think this will work, and it meets your criteria:

• Bump PlotlyJS 0.3.0 to JSON 0.6 minimum version
• Tag JSON 0.5.4 without the separator rename
• Change the (exclusive) upper bounds in this PR from 0.5.2 to 0.6

This will downgrade PlotlyJS 0.3.0 down to 0.2.0 for many, at least until Requests tags a new version.

[TotalVerb]

This revised PR should be merged after/if JSON 0.5.4 is tagged at 0.5.1.

[tkelman]

That sounds workable to me. PlotlyJS needs to put the lower bound in its own REQUIRE otherwise it'll need to be fixed again after every new tag.

[kmsquire]

@tkleman, yeah, sorry, I set 0.5.3 ahead to what I thought were only bug fixes. My bad.

@TotalVerb (and Tony), thanks for being on top of this.

In tagging JSON.jl 0.5.4 at 0.5.1, it would probably be good to make 0.5.2 and 0.5.3 uninstallable.

[tkelman]

I guess we accomplish that with a julia lower bound of 9999 or something like that?

[sglyon]

Sorry for the mixups here and thanks for working on this -- though I think a little more time to let the devs of the affected packages respond would have been better. This felt a bit rushed and intrusive. I don't intend to sound confrontational or upset here so please don't take it that way -- just voicing my thoughts. Overall I'm happy the community stepped in to help clean up my mess.

In PlotlyJS.jl I was basically reimplementing JSON._print for my types for the following reason:

• If I implemented JSON.json(::GenericTrace, ::Int) I could properly serialize one instance of GenericTrace in my custom way.
• However, if I had, say, a Vector{GenericTrace} it would not use my definition of JSON.json. Instead, it seemed to use JSON._print(::Any), which is not my custom serialization and didn't work for my use case. I couldn't tell why that was happening, but didn't want to spend that much time debugging. My bad for being lazy here.
• One way around this problem was to bypass JSON.json and hook directly into JSON._print -- that way I know that my custom serialization is always called.

That all being said, I now realize it is not necessary. I'm going to submit a patch to PlotlyJS that doesn't rely on any internals of JSON.jl (good suggestion @tkelman) I'll tag a new release when I'm done with that.

If you have feelings about whether that should be PlotlyJS v0.3.1 or v0.4.0 please let me know. I'm totally indifferent between the two and unless someone else has strong feelings I'll probably choose v0.3.1.

[TotalVerb]

Your concerns are legitimate. I think your current solution is a very reasonable stopgap. In the long term there will be support from JSON.jl for this use case.

[sglyon]

Ok excellent. Thanks again for working on this.

[tkelman]

Sorry for not waiting a bit longer for your input. Now that PkgEval is running like clockwork and I make a habit of checking it, I tend to notice breakages soon after they happen. Adjusting bounds in metadata is a quick way to get things back from a broken state to a previously working state. New tags can reverse and/or make unnecessary any such changes, but I do think package authors need to be more careful about their dependency version requirements in both directions.

[sglyon]

Happy to hear PkgEval is working again and thanks again for working on this. I'm happy the community is willing to help.

Agreed that more care with bounds on dependencies would be great.