Correctly treat NULL return values from .f() in modify() family

[vspinu]

Fix #753, Fix #655, Fix #746

I am using checking length to see if [] <- list(NULL) modifies length

[lionel-]

This is a bit too complex for me to review right now, but you can use this branch with confidence as we will definitely fix this in the next non-patch release of purrr.

I would use is_list(x) && !is.data.frame(x) instead of is.recursive(), as the latter is too permissive. The former is too permissive as well, but an improvement.

Style-wise, can you please double check the patch is consistent with our style. This includes:

• Wrap single-line branches in {.

• Sentence case for comments, no full stop unless two sentences.

• elt instead of el.

• abort() instead of stop().

•  foo(
     a,
     b,
   )
  

  instead of

  foo(a,
      b)
  

[vspinu]

I would use is_list(x) && !is.data.frame(x)

This would be an unnecessary restriction. In case of data.frames and other containers which don't behave as we expect on [] <- list(NULL) (aka keep same length) we throw with an informative error.

[vspinu]

Ok, applied all you suggestions except for abort and data.frame check as per my comment above. The modify.R still uses stop throughout, please modify all of them yourself as you see fit.

but you can use this branch with confidence as we will definitely fix this in the next non-patch release of purrr.

I could, but there are other people involved and a production system which I don't want to rub needlessly. Could you just pull it for now and deal with the "sophistication" after? The PR is simple, all tests passing, but the bug is critical.