search

markfairbanks / tidytable

Tidy interface to 'data.table'
https://markfairbanks.github.io/tidytable/
Other
450 stars 32 forks source link

Failure with dev glue #788

Closed jennybc closed 10 months ago

jennybc commented 10 months ago

This issue is inspired by doing revdep checks for glue. I'm going to temporarily back off on the associated change in glue, just so I can release without any breakage of other packages.

But please do consider this a heads up that, in the future, glue::glue() will error when .envir is not an actual environment. .envir has always been documented to be an environment and I'd like to make that actually true.

OTOH glue_data() does officially accept something "list-ish" as .x. So chances are you should switch to glue_data(.x = list_ish_thing, ...) as opposed to glue(.envir = list_ish_thing, ...) in one or more places here in this package.

I started to see if I could make a PR for you, but it's not a clear "drive by fix". But I suspect the problematic glue() call has its origins around here:

https://github.com/markfairbanks/tidytable/blob/d2ebb76a0fd6c7502f43733b2ac0eb69f3ebcbc6/R/utils-prep_exprs.R#L110

Backstory in glue:

https://github.com/tidyverse/glue/issues/308 https://github.com/tidyverse/glue/commit/e2b74ff17704261b5a7da25b4ebd2dec94740764

Before I (temporarily) rolled the change back in glue, here's what the tidytable failure look like for me:

https://github.com/tidyverse/glue/blob/c98f106c420d658da8010627d4a2a4d19f97d758/revdep/problems.md#tidytable

markfairbanks commented 10 months ago

Thanks @jennybc I'll get this updated.

FYI that section of code was due to this issue https://github.com/Rdatatable/data.table/issues/4879. Essentially glue() fails in the j position of a [.data.table call unless you specify glue(.envir = .SD). I was updating the call in the background to do that so the user didn't have to. I'll make the switch so it uses glue_data(.x = .SD, ...) instead.

jennybc commented 10 months ago

I released glue 1.7.0 today without this change and then re-introduced the change. So you can expect it to be present in glue's next release, which I have no concrete plans for. But barring an unforeseen release in the next ~2 weeks, I will consider this issue as me having given plenty of notice of the change.

This also means you can check your package in the presence of dev glue to test your own fix for the issue.

markfairbanks commented 10 months ago

Thanks @jennybc. I just verified that the fix works with dev glue. I'll submit a new version of tidytable to CRAN soon to avoid future issues.