Closed DavisVaughan closed 6 years ago
Thanks for the feedback.
id()
is steep. It's a short function name and subconsciously helps to enhance the understanding of key
as identifying variables. Assuming some users have used dplyr::vars()
before, they would find id()
just bind variables together as a helper. lat
and lon
as key
. The key = id()
concept is better to be introduced in the beginning. id()
isn't the first instinct, but I'd like to keep interface consistent and implementation simpler. If there's no explicit key, it's key = id()
suggesting there's a key, but implicit in the data. If changing key
to accept a bare variable without id()
, I'd probably change key = NULL
as default too, but this doesn't make sense to me. Generally speaking, id()
is semantically important to a tsibble.key = id(symbol)
to create tbl_ts
?"This all seems fair to me. If you add the more informative error suggesting exactly what the user should do, I think that would make me happy. Thanks for taking some time to respond.
I think if you can detect that the user input is a name
object, then you could throw that specific error.
Also, just ran into this, which I doubt you want!
library(tsibble)
ex <- data.frame(group = 1:2, date = Sys.Date(), var = .1)
as_tsibble(ex, key = "group")
#> The `index` is `date`.
#> # A tsibble: 2 x 3 [?]
#> # Key: group [2]
#> group date var
#> <int> <date> <dbl>
#> 1 1 2018-09-11 0.1
#> 2 2 2018-09-11 0.1
Created on 2018-09-11 by the reprex package (v0.2.0).
Thanks. All done.
How do you feel about allowing users to pass in the
key
without requiring them to useid()
? I often find myself with just 1 key level, and feel like I should just be able to do:As a new user, I might be confused by the power that
id()
gives me (crossing and nesting) when all i want to do is work with a basictsibble
with 1 layer of keys. You've made it fairly clear in the docs that you need to useid()
, which is good, but it's just not my first instinct. Allowing me to dokey = symbol
and then informing me later on (once my interest is piqued) that I can group by multiple levels and so on withid()
seems slightly more user friendly to me.On first glance, it looks like all that would need to be changed is the
safely(eval_tidy)
call inuse_id()
would need to becomesafely(conditional_eval_tidy)
whereconditional_eval_tidy
could look like:Essentially it just catches the case where the expression returned by
get_expr(key)
is aname
and not acall
. The rest ofuse_id()
would then work fine.