datawookie / emayili

An R package for sending email messages.
https://datawookie.github.io/emayili/
179 stars 27 forks source link

Strip scalar `"header"` class before `hoist()`ing #111

Closed DavisVaughan closed 2 years ago

DavisVaughan commented 2 years ago

We are planning to release tidyr 1.2.0 in mid January.

This package was flagged in our revdeps. You can reproduce the issue quickly by installing the dev version of tidyr and running:

library(emayili)

email <- envelope() %>%
  from("Gerald <gerald@gmail.com>") %>%
  to(c("bob@gmail.com", "alice@yahoo.com")) %>%
  cc("Craig     < craig@gmail.com>") %>%
  bcc("  Erin   <erin@yahoo.co.uk    >")

parties(email)
#> Error: Input must be a vector, not a <header> object.

The problem is with this hoist() call: https://github.com/datawookie/emayili/blob/6b37fdb9629fe2e076b4ad9f56aded342283432f/R/parties.R#L27

Each element of the values column is a "header" object.

Based on this definition of header(), it seems like a header is a scalar object, i.e. it isn't one that should be vectorized (similar to, say, a linear model returned from lm()). https://github.com/datawookie/emayili/blob/a73b423a0d0de57ba3d40219ee1cbc12907da37b/R/header.R#L1-L14

Because it is a scalar, our vctrs related tooling (which hoist() uses) doesn't know how to work with it (it is designed to work with vectors). Since you seem to want to treat it like a vector list here in this hoist() call, I recommend unclassing the header objects before calling hoist(). That is what this PR does.

Alternatively, if you do want "header" to be treated like a list in all cases, rather than like a scalar object, then if you change this class from class = "header" to class = c("header", "list") then vctrs will treat it like a list instead. In that case, you could close this PR and make that change instead. https://github.com/datawookie/emayili/blob/a73b423a0d0de57ba3d40219ee1cbc12907da37b/R/header.R#L12

We'd greatly appreciate if you could apply this fix and send a new version of your package out in early January so we can release tidyr with minimal issues. Thank you!

datawookie commented 2 years ago

Hi @DavisVaughan,

Thanks very much for this detailed merge request. I was happy to accept it. However, with subsequent testing I found that there was another issue with the address class. So I have simplified the implementation of the address class (removing use of the {vctrs} package altogether.

I really appreciate your diligence with this.

Best regards, Andrew.