Strict pluck #482 #522

daniel-barnett · 2018-07-10T01:40:36Z

This is an implementation of strict pluck, so-called chuck from #482.

Once we have agreed on everything, I'll also add some examples and descriptions to the help.

Since this is based on pluck, it seems like #480 is involved here too.

hadley

Overall, looks good, although I think it would be better to move the checking a bit further down in order to get better error messages (which will require some test tweaking).

hadley · 2018-08-27T15:26:16Z

src/extract.c

@@ -66,10 +66,17 @@ int find_offset(SEXP x, SEXP index, int i) {
  }
 }

-SEXP extract_vector(SEXP x, SEXP index_i, int i) {
+SEXP extract_vector(SEXP x, SEXP index_i, int i, int strict) {
  int offset = find_offset(x, index_i, i);


I think it might be better to pass strict down to find_offset() — that's obviously a lot more work because there are so many more returns, but I think it would lead to considerably more informative error messages.

I've adjusted all of this so it gives specific messages for each type of problem in find_offset(); however, for the error messages, which way do you think is better out of one or two line errors?

chuck(1:10, Inf) #> Error: Each accessor must be finite: #> * Accessor 1 is Inf chuck(1:10 Inf) #> Error: Accessor 1 must be finite, not Inf

The second - I slightly prefer the wording, and the former suggests we might warn for any Inf, not just the first we encounter (which while nice, I don't think that would be worth the implementation complexity)

hadley · 2018-08-27T15:28:22Z

src/extract.c

 }

-SEXP extract_attr(SEXP x, SEXP index_i, int i) {
+SEXP extract_attr(SEXP x, SEXP index_i, int i, int strict) {


Maybe should rename to extract_s4() to make the intent more clear?

hadley · 2018-08-27T15:29:03Z

src/extract.c

+      Rf_errorcall(R_NilValue,
+        "Can't find slot `%s`.",
+        Rf_translateCharUTF8(Rf_asChar(index_i))
+      );


Feels like you could use an else block here, just to make clear what the default is (instead of relying on Rf_getAttrib())

daniel-barnett · 2018-09-12T05:24:53Z

Sorry for the delay; this latest push should have more informative error messages as I moved it further down as you suggested. I did change some of existing error messages (i.e. the ones that pluck also uses) so they're more consistent. If this is fine, I can go ahead and write up something for the documentation.

hadley

Looking good! A couple more comments/questions

hadley · 2018-09-12T12:26:28Z

src/extract.c

+    } else if (val >= n) {
+      if (strict)
+        Rf_errorcall(R_NilValue,
+          "Index %i exceeds the length of object being plucked (%.0f > %i).",


Why %.0f and not %i?

Because of double val = REAL(index)[0];, using %i gives nvalid index (1): index must be greater than 0, not 536870913..

hadley · 2018-09-12T12:33:03Z

src/extract.c

  if (Rf_length(index) > 1) {
-    Rf_errorcall(R_NilValue, "Index %i must have length 1", i + 1);
+    Rf_errorcall(R_NilValue,
+      "Index %i must have length 1, not %i.",


I think these messages are a bit easier to write/read if they all start in the same way, either with "Invalid object:" or "Invalid index (%i):".

Also can you replace all instances of "must not" by "can't" please? For instance this:

Object being plucked must not be NULL.

reads better as:

Plucked object can't be NULL.

cf https://style.tidyverse.org/error-messages.html

(just to be clear, the affirmative "must" cases are good!)

hadley · 2018-09-12T12:36:05Z

Feel free to start on the documentation - I think you should document chuck() with pluck() (i.e. with @rdname chuck, and then just tweak the existing pluck docs)

lionel- · 2018-11-15T18:19:43Z

src/extract.c

-    if (val < 0 || val >= n)
-      return -1;
+    if (val < 0) {
+      if (strict)


General comment: can you add curly braces even to single expressions please?

Did you want me to add those for the existing code that I haven't yet modified too?

@lionel- I don't think we use that style anymore, and no parens is fine for simple expressions.

Ah I wasn' aware, Jim, Gabor and I still use it. It's consistent with our R style and easier to read since we use 2-level indents. Especially with multi-line single expressions such as here. But nevermind then.

It can also have implications for covr, I think? That is, the presence of the brackets helps covr do its job in some weird edge cases.

I thought the edge cases were fixed and this is officially blessed @jimhester style

Maybe I am behind the times then.

The officially blessed jimhester style is to always use braces for single expressions.

The edge cases in covr are fixed, but I think it is still better to use braces, it is too easy to accidentally forget to add the braces in the future and write something like the following, then not realize the second if is unconditionally evaluated.

if (val < 0) print(val) if (strict)

hadley · 2018-11-19T13:28:06Z

@daniel-barnett do you mind updating your code to use {} for all single expressions? Don't worry about changing the style for any code that you didn't write. We'll fix that.

lionel- · 2018-11-27T09:52:27Z

Don't worry about the latest comments @daniel-barnett, I'm going to finish the PR because I need it merged before working on other pluck bugs and features.

The latter wrongly suggests something is going on with different literal types

Closes #482 Closes #550

lionel- · 2018-11-27T14:15:16Z

Refactored the code to put all the checking logic at the end of the file. Merged several code paths to get more consistent error checking and make it easier to maintain. Improved consistency of error messages.

@hadley Could you review the last commit which adds documentation please? I have moved attr_getter() in its own topic so as not to distract from the important stuff.

hadley

Just reviewed the docs

hadley · 2018-11-27T16:55:45Z

R/pluck.R

-#' accessor functions, i.e. functions that take an object and return
-#' some internal piece.
+#' `pluck()` and `chuck()` implement a generalised form of `[[` that
+#' allow you to index deeply and flexibly into data structures. While


Remove "while"

hadley · 2018-11-27T16:56:16Z

R/pluck.R

-#' accessors because it reads linearly and is free of syntactic
-#' cruft. Compare: \code{accessor(x[[1]])$foo} to `pluck(x, 1,
-#' accessor, "foo")`.
+#' * You can pluck or chuck with standard accessors like integer


Maybe these should move down to details?

hadley · 2018-11-27T16:56:34Z

R/pluck.R

+#'
+#' @details
+#'
+#' Since it handles arbitrary accessor functions, `pluck()` is a type


I don't understand this paragraph. Maybe it can be removed?

lionel- · 2018-11-27T20:02:49Z

Thanks again @daniel-barnett !

hadley reviewed Aug 27, 2018

View reviewed changes

hadley reviewed Sep 12, 2018

View reviewed changes

lionel- reviewed Nov 15, 2018

View reviewed changes

daniel-barnett and others added 17 commits November 27, 2018 15:05

Add chuck() as a strict variant of pluck()

d19e803

Adjust tests now that chuck() no longer auto-splices

ae33c46

Use backticks in error messages

ab4e5b8

Use "can't" instead of "must not" in error messages

0ce0ea8

Wrap branches with curly braces in extract.c

08f89b3

Use switch instead of cascading if-else

116a700

Replace "object being plucked" with "plucked object"

38b574e

Rename extract.c to pluck.c

feed03f

Use bool type in pluck.c

4027eb6

Extract pluck checks in separate functions

1e9d00f

Use same code path as other types with NULL

210e670

Dispatch on sexptype in find_offset()

7dd29d8

Merge integer and double index code paths

c0384cf

Use %d specifier instead of %i

50f5ba9

The latter wrongly suggests something is going on with different literal types

Move checking functions at end of file

68ad335

Tweak error message style

344bbba

Review documentation of pluck(), chuck(), and attr_getter()

94572ed

Closes #482 Closes #550

hadley approved these changes Nov 27, 2018

View reviewed changes

Reorganise details section of ?pluck

8b124ee

lionel- merged commit b4ae036 into tidyverse:master Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strict pluck #482 #522

Strict pluck #482 #522

daniel-barnett commented Jul 10, 2018

hadley left a comment

hadley Aug 27, 2018

daniel-barnett Sep 1, 2018

hadley Sep 1, 2018

hadley Aug 27, 2018

hadley Aug 27, 2018

daniel-barnett commented Sep 12, 2018

hadley left a comment

hadley Sep 12, 2018

daniel-barnett Oct 7, 2018

hadley Oct 7, 2018

hadley Sep 12, 2018

lionel- Nov 16, 2018

hadley commented Sep 12, 2018

lionel- Nov 15, 2018

daniel-barnett Nov 15, 2018

hadley Nov 15, 2018

lionel- Nov 16, 2018

jennybc Nov 16, 2018

hadley Nov 16, 2018

jennybc Nov 16, 2018

jimhester Nov 16, 2018

hadley Nov 19, 2018

hadley commented Nov 19, 2018

lionel- commented Nov 27, 2018

lionel- commented Nov 27, 2018

hadley left a comment

hadley Nov 27, 2018

hadley Nov 27, 2018

hadley Nov 27, 2018

lionel- commented Nov 27, 2018

Strict pluck #482 #522

Strict pluck #482 #522

Conversation

daniel-barnett commented Jul 10, 2018

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-barnett commented Sep 12, 2018

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented Nov 19, 2018

lionel- commented Nov 27, 2018

lionel- commented Nov 27, 2018

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lionel- commented Nov 27, 2018