Fixes #1126

[Mattriks]

• I've updated the documentation to reflect these changes
• I've run the regression tests
• I've squash'ed or fixup'ed junk commits with git-rebase
• I've built the docs and confirmed these changes don't cause new errors

Proposed changes

This PR (closes #1126):

• deprecates thelowlight_opacity argument in Theme, and adds a depwarn.
• in the docs, updates Geom.ribbon and shows how to use Theme(lowlight_color=) to add color transparency to Geom.ribbon.
• enables Theme(lowlight_color=) for Geom.polygon. This makes plot 2 below possible:

using Colors, DataFrames, Distributions, Gadfly
x = -4:0.1:4
Da = [DataFrame(x=x, ymax=pdf.(Normal(μ),x), ymin=0.0, u="μ=$μ") for μ in [-1,1]]
Db = [DataFrame(x=randn(200)+μ, u="μ=$μ") for μ in [-1,1]] 

p1 = plot(vcat(Da...), x=:x, y=:ymax, ymin=:ymin, ymax=:ymax, color=:u, 
    Geom.line, Geom.ribbon, Guide.ylabel("Density"),
    Theme(lowlight_color=c->RGBA{Float32}(c.r, c.g, c.b, 0.4)), 
    Guide.colorkey(title="", pos=[2.5,0.6]), Guide.title("Parametric PDF")
)
p2 = plot(vcat(Db...), x=:x, color=:u, 
    Stat.density(bandwidth=0.5), Geom.polygon(fill=true, preserve_order=true),
    Coord.cartesian(xmin=-4, xmax=4),
    Theme(lowlight_color=c->RGBA{Float32}(c.r, c.g, c.b, 0.4)),
    Guide.colorkey(title="", pos=[2.5,0.6]), Guide.title("Kernel PDF")
)
hstack(p1,p2)

I added this example in the docs under Stat.density.

[bjarthur]

any idea why tests are failing on shapekey?

also, given the removal of a field from Theme, we should probably bump the minor version when this is merged. any other breaking changes we want to include in such a release? maybe #1118 ?

otherwise, looks great!

[Mattriks]

The Theme() constructor produced by the @varset macro allows strings to be parsed to colors. You can tell this by doing:

th = Theme(key_swatch_color="slategrey")
dump(th.key_swatch_color)

In the Theme outer constructor that I added in this PR, the field type e.g. ColorOrNothing is hard-coded, so Theme(key_swatch_color="slategrey") fails! Removing the Type of the keyword arguments should fix it, but I find that ugly. Maybe just not including a depwarn is better?

[bjarthur]

gadfly has 1000+ stars so i think it's important to document breaking changes with a depwarn. they can be removed after a release or two, so the ugliness is only temporary.

maybe Union{ColorOrNothing,String} ?

see #998

[bjarthur]

i think you just need to add parse_colorant to #1156

[Mattriks]

Still trying to understand how to do this deprecation in a simple way. In order for Theme(default_color="red") to work, the Theme constuctor (containing any deprecations) needs to also be able to parse a string to colorant etc. Note that style(default_color="red") does not work for the same reason i.e. style does not do any parsing of its kwargs before passing them to Theme.

[bjarthur]

would Theme(default_color==nothing ? nothing : parse_colorant(default_color), .... work? this is how it's done for the non-deprecated constructor https://github.com/GiovineItalia/Gadfly.jl/blob/master/src/varset.jl#L47

[Mattriks]

But that's no different then making a new Theme constructor, in which case the varset constructor for Theme is obsolete. If the Theme constructor is coded explicitly then yes it easy to do deprecations. Removing the varset Theme constructor and making a new constructor should really be done as a separate PR.

[bjarthur]

i think all we need here is

function Theme(; default_color, point_size, ..., lowlight_opacity, ...)
    Base.depwarn(....)
    Theme(
        default_color==nothing ? nothing : parse_colorant(default_color),
        point_size,
        ...
        #skip lowlight_opacity,
        ...)
end

no need to specify the types, as that will be handled by the varset definition of the Theme object.

[codecov-io]

Codecov Report

Merging #1175 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
+ Coverage   87.32%   87.33%   +<.01%     
==========================================
  Files          34       34              
  Lines        4080     4082       +2     
==========================================
+ Hits         3563     3565       +2     
  Misses        517      517

Impacted Files	Coverage Δ
src/geom/polygon.jl	66.66% <0%> (ø)	⬆️
src/theme.jl	68.51% <100%> (+1.21%)	⬆️

---

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 fcf7596...4542f3f. Read the comment docs.

[Mattriks]

Added a Theme outer constructor for doing argument deprecations, rebased and updated docs.

[tlnagy]

I'm okay with merging these changes now.

[bjarthur]

next tag will need to be a minor not point release since this introduces a breaking change.

[bjarthur]

for some reason this PR is causing a warning, and i don't understand why.

julia> using Gadfly
WARNING: Method definition Type(Array{Any, 1}, Type{Gadfly.Theme}) in module Gadfly overwritten.
WARNING: Method definition (::Type{Gadfly.Theme})() in module Gadfly at /Users/arthurb/.julia/v0.6/Gadfly/src/varset.jl:62 overwritten at /Users/arthurb/.julia/v0.6/Gadfly/src/theme.jl:473.

the Theme constructor with the depwarn in it should be equivalent to the code below, but the latter doesn't emit the same warning.

julia> struct Foo
       a
       end

julia> Foo(; a=1) = Foo(a)
Foo

julia> Foo(; a=1, b=2) = Foo(a)
Foo

[tlnagy]

I'm pretty sure this has something to do with varset creating a constructor and then the explicit constructor in theme.jl overwrites it. Might be better to do the deprecation in @varset, i.e. add an option to the macro to deprecate a field.

[bjarthur]

think i've got a fix #1180