Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide suggestions for unknown kwargs passed to blocks #4392

Merged
merged 13 commits into from
Dec 5, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- Show DataInspector tooltip on NaN values if `nan_color` has been set to other than `:transparent` [#4310](https://github.com/MakieOrg/Makie.jl/pull/4310)
- Fix `linestyle` not being used in `triplot` [#4332](https://github.com/MakieOrg/Makie.jl/pull/4332)
- Invalid keyword arguments for `Block`s (e.g. `Axis` and `Colorbar`) now throw errors and show suggestions rather than simply throwing [#4392](https://github.com/MakieOrg/Makie.jl/pull/4392)
- Fix voxel clipping not being based on voxel centers [#4397](https://github.com/MakieOrg/Makie.jl/pull/4397)

## [0.21.11] - 2024-09-13
Expand Down
24 changes: 15 additions & 9 deletions MakieCore/src/recipes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -706,27 +706,33 @@ function print_columns(io::IO, v::Vector{String}; gapsize = 2, rows_first = true
return
end

__obj_name(_) = "plot"
__valid_attributes(p) = attribute_names(p)
__has_generic_attributes(_) = true

function Base.showerror(io::IO, i::InvalidAttributeError)
n = length(i.attributes)
print(io, "Invalid attribute$(n > 1 ? "s" : "") ")
for (j, att) in enumerate(i.attributes)
j > 1 && print(io, j == length(i.attributes) ? " and " : ", ")
printstyled(io, att; color = :red, bold = true)
end
print(io, " for plot type ")
print(io, " for $(__obj_name(i.plottype)) type ")
printstyled(io, i.plottype; color = :blue, bold = true)
println(io, ".")
nameset = sort(string.(collect(attribute_names(i.plottype))))
nameset = sort(string.(collect(__valid_attributes(i.plottype))))
println(io)
println(io, "The available plot attributes for $(i.plottype) are:")
println(io, "The available $(__obj_name(i.plottype)) attributes for $(i.plottype) are:")
println(io)
print_columns(io, nameset; cols = displaysize(stderr)[2], rows_first = true)
allowlist = attribute_name_allowlist()
println(io)
println(io)
println(io, "Generic attributes are:")
println(io)
print_columns(io, sort([string(a) for a in allowlist]); cols = displaysize(stderr)[2], rows_first = true)
if __has_generic_attributes(i.plottype)
allowlist = attribute_name_allowlist()
println(io)
println(io)
println(io, "Generic attributes are:")
println(io)
print_columns(io, sort([string(a) for a in allowlist]); cols = displaysize(stderr)[2], rows_first = true)
end
println(io)
end

Expand Down
22 changes: 22 additions & 0 deletions src/makielayout/blocks.jl
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,27 @@
return attributes
end

MakieCore.__obj_name(::Type{<:Block}) = "block"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite dislike _funcname and I like __func_name even less :D
It seems like all of the __ functions could be easily replaced as well by just x isa Plot or attribute_names?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about isa since I don't know what else there is besides x isa Plot and x isa Block. I could do x isa Plot though.

The __valid_attributes is because I didn't know if it made sense to just define attribute_names(::Type{<:Block}). Does it? It could replace __valid_atributes(::Type{<:Block}) below

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attribute_names(::Type{<:Block}) sounds good to me, if @jkrumbiegel doesn't see a problem with it ;)
For the __obj_name, isn't that just string(block_or_plot_type)? I wouldn't mind it being a bit more verbose in favor of it being more correct and not introducing a new function for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried string initially but for some plot types like Triplot it shows all the parameters and takes up so much space. Is there a better way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?

x = typeof(plot_or_block)
string(x <: Plot ? Makie.plotsym(x) : x)

I'd just do it like this, but I'd also be happy to have a function for this, even as an interface without any _ ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I knew about plotsym. I'll look into all of this soon, thanks.

function MakieCore.__valid_attributes(T::Type{S}) where {S<:Block}
attrs = _attribute_docs(T)

Check warning on line 293 in src/makielayout/blocks.jl

View check run for this annotation

Codecov / codecov/patch

src/makielayout/blocks.jl#L291-L293

Added lines #L291 - L293 were not covered by tests
# Some blocks have keyword arguments that are not attributes.
# TODO: Refactor intiailize_block! to just not use kwargs?
(S <: Axis || S <: PolarAxis) && (attrs[:palette] = "")
S <: Legend && (attrs[:entrygroups] = "")
S <: Menu && (attrs[:default] = "")
S <: LScene && (attrs[:scenekw] = "")
return keys(attrs)

Check warning on line 300 in src/makielayout/blocks.jl

View check run for this annotation

Codecov / codecov/patch

src/makielayout/blocks.jl#L296-L300

Added lines #L296 - L300 were not covered by tests
end
MakieCore.__has_generic_attributes(::Type{<:Block}) = false

Check warning on line 302 in src/makielayout/blocks.jl

View check run for this annotation

Codecov / codecov/patch

src/makielayout/blocks.jl#L302

Added line #L302 was not covered by tests

function _check_remaining_kwargs(T::Type{<:Block}, kwdict::Dict)
badnames = setdiff(keys(kwdict), MakieCore.__valid_attributes(T))
if !isempty(badnames)
throw(MakieCore.InvalidAttributeError(T, badnames))

Check warning on line 307 in src/makielayout/blocks.jl

View check run for this annotation

Codecov / codecov/patch

src/makielayout/blocks.jl#L304-L307

Added lines #L304 - L307 were not covered by tests
end
return

Check warning on line 309 in src/makielayout/blocks.jl

View check run for this annotation

Codecov / codecov/patch

src/makielayout/blocks.jl#L309

Added line #L309 was not covered by tests
end

function _block(T::Type{<:Block}, fig_or_scene::Union{Figure,Scene}, args, kwdict::Dict, bbox; kwdict_complete=false)

# first sort out all user kwargs that correspond to block attributes
Expand All @@ -301,6 +322,7 @@
end
# the non-attribute kwargs will be passed to the block later
non_attribute_kwargs = kwdict
_check_remaining_kwargs(T, non_attribute_kwargs)

Check warning on line 325 in src/makielayout/blocks.jl

View check run for this annotation

Codecov / codecov/patch

src/makielayout/blocks.jl#L325

Added line #L325 was not covered by tests

topscene = get_topscene(fig_or_scene)
# retrieve the default attributes for this block given the scene theme
Expand Down
45 changes: 44 additions & 1 deletion test/pipeline.jl
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,13 @@ end
@test all(x -> x isa Volume, plots)
end

import Makie.MakieCore: InvalidAttributeError
import Makie.MakieCore:
__obj_name,
__valid_attributes,
__has_generic_attributes,
InvalidAttributeError,
attribute_names
import Makie: _attribute_docs

@testset "validated attributes" begin
@test_throws InvalidAttributeError heatmap(zeros(10, 10); does_not_exist = 123)
Expand Down Expand Up @@ -176,3 +182,40 @@ end
@test_throws InvalidAttributeError testrecipe(1:4, 1:4, colour=:red)
@test testrecipe(1:4, 1:4, color=:red) isa Makie.FigureAxisPlot
end

@testset "validated attributes for blocks" begin
@test __obj_name(Lines) == "plot"
@test __valid_attributes(Lines) == attribute_names(Lines)
@test __has_generic_attributes(Lines)

@test __obj_name(Axis) == "block"
@test __valid_attributes(Axis3) == keys(_attribute_docs(Axis3))
@test !__has_generic_attributes(Axis3)

fig = Figure()
@test_throws InvalidAttributeError Axis(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Axis3(fig[1, 1], does_not_exist = 123, does_not_exist2 = 123)
@test_throws InvalidAttributeError lines(1:10, axis = (does_not_exist = 123,))
@test_throws InvalidAttributeError Colorbar(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Label(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Box(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Slider(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError SliderGrid(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError IntervalSlider(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Button(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Toggle(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Menu(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Legend(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError LScene(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError Textbox(fig[1, 1], does_not_exist = 123)
@test_throws InvalidAttributeError PolarAxis(fig[1, 1], does_not_exist = 123)

@test Axis(fig[1, 1], palette = nothing) isa Axis # just checking that it doesn't error
@test Menu(fig[1, 2], default = nothing) isa Menu
@test Legend(fig[1, 3], entrygroups = []) isa Legend
@test PolarAxis(fig[1, 4], palette = nothing) isa PolarAxis
@test :palette in __valid_attributes(Axis)
@test :default in __valid_attributes(Menu)
@test :entrygroups in __valid_attributes(Legend)
@test :palette in __valid_attributes(PolarAxis)
end
Loading