Define JSON.lower(CatValue) if JSON is available

[alyst]

Uses Requires.jl to define JSON.json(::CatValue) if JSON is present.

Fixes #95

[ararslan]

I'm very much against adding a dependency on Requires in general, but especially in a package so far down the stack.

[nalimilan]

Yes, I'd rather wait until we have optional dependencies (IIUC they should be supported in Julia 0.7).

[ararslan]

IIUC they should be supported in Julia 0.7

??? As of when?

[alyst]

Is there a proper way to optionally define JSON.lower() in v0.6? If there's no, I think the issue is quite serious and potentially wide-spread to add JSON.jl to REQUIRE (it depends only on Compat).

Requires.jl was the first thing I found, but if its usage is discoraged, maybe @MikeInnes can add a note to README.md?

[ararslan]

The use of Requires isn't discouraged in general; some packages and ecosystems prefer to use it, but the JuliaStats and JuliaData packages avoid it. It's a matter of preference and consistency.

Is there a proper way to optionally define JSON.lower() in v0.6?

No

[nalimilan]

??? As of when?

I mean, when it will be released. :-)

I guess we could add a dependency on JSON until then, but that's not great and that would create a precedent for other dependencies. Last time we added a dependency on a lightweight Juno package for DataFrames, we ended up reverting it, so... Do you have an actual need for this?

[alyst]

Do you have an actual need for this?

Sometimes I'm just saving dataframes to json, but today I have stumbled over it while trying to draw a plot using PlotlyJS (scatter(dataframe, x=:somecategoricalcolumn, ...)).
So this issue may potentially appear in many unexpected contexts.

[alyst]

As an alternative, maybe something along these lines: CategoricalValue uses Ref{CategoricalPool} to refer to the pool, while JSON.lower(x::Ref) (defined in JSON itself) just prints the address of x.

[nalimilan]

That's clever, but that could slow down some essential operations, so that doesn't sound great.

[nalimilan]

Given that this makes PlotlyJS unusable with DataFrames with categorical columns, I guess we should add the dependency on JSON for now (without Requires).

BTW, special support for CategoricalArrays will be needed so that the ordering of levels is respected when plotting. See similar issues: plotly/plotly.js#189, vega/altair#245 and vega/vega-lite#2915.

[codecov]

Codecov Report

Merging #96 into master will decrease coverage by 0.2%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
- Coverage   97.78%   97.58%   -0.21%     
==========================================
  Files           9       10       +1     
  Lines         678      662      -16     
==========================================
- Hits          663      646      -17     
- Misses         15       16       +1

Impacted Files	Coverage Δ
src/CategoricalArrays.jl	0% <ø> (ø)
src/value.jl	93.02% <0%> (-1.03%)	⬇️
src/recode.jl	100% <0%> (ø)	⬆️
src/extras.jl	98.07% <0%> (ø)	⬆️
src/subarray.jl	100% <0%> (ø)	⬆️
src/pool.jl	97.95% <0%> (ø)	⬆️
src/missingarray.jl
src/nullablearray.jl	100% <0%> (ø)
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa30eea...91e9676. Read the comment docs.

[ararslan]

I don't think we should use Requires here.

[alyst]

JSON is a hard dependency now. CI failures seem unrelated

[nalimilan]

Thanks. Let's go with that since there's no other solution at this point...

Concern addressed

[nalimilan]

Now that Requires.jl is no longer a hack, I've used it to get rid of the dependency on JSON: #163.

[nalimilan]

Actually, it turns out that Requires imposes a significant penalty on load time, which people complain about (JuliaData/DataFrames.jl#1764). On my machine, with Julia master, using JSON takes 0.05s, while using Requires takes 0.19s. So I wonder whether it wouldn't be better to use a hard dependency for now. (But it's still really annoying that we have to load JSON at all. Hopefully at some point it will be possible to define methods without loading a package.)

[ararslan]

Yeah, Requires may no longer be a hack, but it still isn't a great solution. I'm fine with a dependency on JSON, as it's a common package to have installed anyway.

[nalimilan]

OK, see #189.