tidyverse / ggplot2

An implementation of the Grammar of Graphics in R
https://ggplot2.tidyverse.org
Other
6.48k stars 2.02k forks source link

date_breaks and date_breaks minor don't check argument type #5880

Open botanize opened 5 months ago

botanize commented 5 months ago

datetime_scales accepts any argument type for date_breaks and date_minor_breaks, but the parameter documentation claims it only accepts character strings. It's somewhat easy to accidentally supply custom date breaks using scales::breaks_width to the wrong parameter (date_breaks instead of breaks), after all, you're defining date-breaks either way.

I expected an error that the date_breaks parameter had the wrong type, instead I got the unhelpful: Error in strsplit(unitspec, " ") : non-character argument.

ggplot(economics, aes(date, unemploy)) + 
  geom_line() + 
  scale_x_date(date_breaks = scales::breaks_width('10 years', offset = '6 months')
teunbrand commented 5 months ago

Thanks for the report! If this is a request for better messages, it is probably better to appeal to {scales} to improve the message. ggplot2 has little control over the messages the packages it depend on throw.

botanize commented 5 months ago

I would like better messages, but I think the best way of doing that would be to check that the inputs received by the function are the correct type (as stated by function documentation), instead of passing them blindly to other functions unknown. In this case, it's because I passed a function to the wrong breaks parameter (date_breaks instead of breaks). While I guess {scales} could check inputs as well, this does seem like something that should happen in datetime_scales. Oddly, I see that the default value for the date_breaks parameter in datetime_scales is waiver(), clearly not a character string.