-
Notifications
You must be signed in to change notification settings - Fork 8
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
Pass type arguments through dots #267
Conversation
This seems pretty straightforward and useful. We may still want to strictly advertise the "old" system on the website and in the docs, because it clearly indicates where users should look for to get documentation on type-specific arguments. Perhaps showing a call-out box with a "tip" about Not quite the "there should be only one way" Tao, but... |
I've now also added |
I've looked at the code and don't see any problem. Do you plan to do more work on this? If not, I think we should just merge it. |
Great, thanks for the feedback, Vincent. I also don't think that further work is needed. So let's see whether Grant @grantmcdermott has any concerns... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Achim. I agree that this is helpful and adds a disciplined "backend" for users that would like to pass customization arguments without having to go through the type_*()
functions.
On that note, we'll also need to edit the first "breaking changes" item in NEWS here.
Lines 9 to 25 in d76f4f1
Breaking changes: | |
- As part of our new plot `type` logic (see below), character-based shortcuts | |
like `type = "p"`, `type = "hist"`, etc. are reserved for default behaviour. | |
In turn, this means that any type-specific arguments for customizing behaviour | |
should no longer be passed through the main `tinyplot(...)` function (i.e., | |
via `...`), since they will be ignored. Rather, these ancilliary arguments | |
must now be passed explicitly as part of the corresponding `type_*()` function | |
to the `type` argument. For example, say that you want to change the default | |
number of breaks in a histogram plot. Whereas previously you could have called, | |
say, `tinyplot(Nile, type = "hist", breaks = 30)`, now you should instead | |
call `tinyplot(Nile, type = type_hist(breaks = 30))`. We're sorry for | |
introducing a breaking change, but again this should only impact plots that | |
deviate from the default behaviour. Taking a longer-term view, this new `type` | |
logic ensures that users can better control how their plots behave, avoids | |
guesswork on our side, and should help to reduce the overall maintenance burden | |
of the package. |
Do you mind taking a crack at the re-wording? We can obviously move it out of breaking changes, since the old way will work again with your PR. But we should underscore that the documentation lives with the individual type functions and politely suggest that passing arguments through them (rather than
...
) is the more idiomatic tinyplot way of doing things now.
EDIT: We should also update the documentation here to note that extra type-specific args can be passed through ...
.
Line 301 in d76f4f1
#' @param xaxs,yaxs,... other graphical parameters (see \code{\link[graphics]{par}}). |
R/sanitize.R
Outdated
if (is.character(type)) type = switch(type, | ||
"points" = type_points, | ||
"segments" = type_segments, | ||
"area" = type_area, | ||
"rect" = type_rect, | ||
"polypath" = type_polypath, | ||
"polygon" = type_polygon, | ||
"pointrange" = type_pointrange, | ||
"errorbar" = type_errorbar, | ||
"boxplot" = type_boxplot, | ||
"ribbon" = type_ribbon, | ||
"histogram" = type_histogram, | ||
"spineplot" = type_spineplot, | ||
"qq" = type_qq, | ||
"j" = type_jitter, | ||
"jitter" = type_jitter, | ||
"loess" = type_loess, | ||
"ridge" = type_ridge, | ||
"spline" = type_spline, | ||
"glm" = type_glm, | ||
"lm" = type_lm, | ||
"function" = type_function, | ||
type # Default case | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something obvious here, but shouldn't type_lines()
be included too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related could we organise this list of switch elements alphabetically, so that it's easier to keep track of new/missing types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rephrased/reorganized the NEWS. Please have a look at whether you like this approach or would prefer to do it differently. Maybe there should also be something in the vignette?
I also noticed that there is still a mentioning of ribbon.apha
in tinyplot.Rd
which can probably go.
I fully agree with the other proposed changes and will do them a bit later...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that there is still a mentioning of
ribbon.apha
intinyplot.Rd
which can probably go.
Good catch! Yes, please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done. Please check the NEWS update to see whether you like it - and whether you agree that the non-standard spacing is helpful in https://github.com/grantmcdermott/tinyplot/blob/type-dots/R/sanitize.R#L36-L64
Open: Add something in vignette? Add tests, e.g., checking that tinyplot(Nile, type = "hist", breaks 30)
is indeed identical to tinyplot(Nile, type = type_hist(breaks = 30))
?
Great stuff, thanks @zeileis! |
Today I took a quick stab at passing arguments through the dots
...
to thetype_*
function. The main motivation for this came from #233 (comment). Because:works, I also wanted to get
to work rather than having to call
Similarly, instead of
we can now also do
rather than having to use
I've implemented this by:
dots
also tosanitize_type
.type
function without evaluating it yet.dots
that match arguments oftype
using them to evaluatetype
.dots
back to thetinyplot
.Thus, this only works for
type
arguments that are not arguments oftinyplot
_and_ do not even partially match! So this is a potential source of confusion. But it does avoid the potential confusion from examples like the ones above.