Provide suggestions for unknown kwargs passed to blocks #4392
Conversation
|
Thanks for the PR! I don't think this qualifies as breaking if it's just changing the type of the error? Any situation that triggers this would have errored anyway. |
|
Right. I was thinking of the case where kwargs used to get silently ignored in recipes but that's not the case here. |
hehe...from the docs build |
|
function MakieCore.__valid_attributes(T::Type{<:Legend})
atrs = _attribute_docs(Legend)
atrs[:entrygroups] = ""
return keys(atrs)
endunless someone can comment on what's going on with |
|
There are still some issues with the tests with this approach. I am guessing my method of determining the valid attributes is just not great, e.g. |
|
I think given that there are only one or two exceptions to the rule that Blocks have no further kwargs than their attributes, and it's not sure if that's actually necessary, it would be worth refactoring that mechanism such that initialize never takes kwargs. To me the feedback mechanism for wrong usage is much more important than these edge cases. If they cannot be removed completely. |
|
Should that refactoring be handled in this PR? Currently I'm just trying to give a bandaid fix to my current solution but it could be pretty simple to refine it later. |
|
I haven't checked yet what your solution is, but maybe these kwarg uses can just be converted to attributes. |
|
I'll try and get that implemented. |
|
There are a bunch of annoying parts here that I don't understand when trying to convert the kwargs to just be attributes. I think it would be simpler for this PR to just leave it as is and then come back to this issue at another time? |
|
Sure, so what are you doing with those keywords now? |
function MakieCore.__valid_attributes(T::Type{S}) where {S<:Block}
attrs = _attribute_docs(T)
# 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)
endThey are just getting added into the available attributes (this function is only used in that error message) |
src/makielayout/blocks.jl
Outdated
| @@ -288,6 +288,27 @@ function block_defaults(::Type{B}, attribute_kwargs::Dict, scene::Union{Nothing, | |||
| return attributes | |||
| end | |||
|
|
|||
| MakieCore.__obj_name(::Type{<:Block}) = "block" | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 _ ;)
There was a problem hiding this comment.
I don't think I knew about plotsym. I'll look into all of this soon, thanks.
|
Just a heads up that I probably won't get around to finishing this one up either to properly address your suggestions @SimonDanisch sorry, or at least not for quite a while (at which point I might have forgotten about this anyway 😅 ) now that I've got too much other stuff going on. |
|
I replaced the __ functions:
|
Description
Fixes #4391. There might be a better way to do what I'm doing here, but at least it's something to test with instead of my code snippets on that issue.
Adds a check to block constructors to see if the passed kwargs are all valid. Reuses the
InvalidAttributeErrorcode. Some examples:I wanted to get #4390 here too, but
Figureisn't a block... the bad keyword arguments are just passed intoScene, e.g.Not sure if that's a simple fix. I've never used themes. So, I won't address it here and keep it simple.
Type of change
Checklist