From 9a4929070213626a520025c4570929e27eb3f431 Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Wed, 9 May 2018 16:41:01 -0700
Subject: [PATCH 1/9] `$` no longer attempts to use __getitem__

---
 R/python.R | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/R/python.R b/R/python.R
index 6382f38d5..68135cd3d 100644
--- a/R/python.R
+++ b/R/python.R
@@ -243,9 +243,7 @@ py_has_convert <- function(x) {
   }
 
   # get the attrib
-  if (is.numeric(name) && (length(name) == 1) && py_has_attr(x, "__getitem__"))
-    attrib <- x$`__getitem__`(as.integer(name))
-  else if (inherits(x, "python.builtin.dict"))
+  if (inherits(x, "python.builtin.dict"))
     attrib <- py_dict_get_item(x, name)
   else
     attrib <- py_get_attr(x, name)

From ecb99221a133ad90c54d1212f027993545323c02 Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Wed, 9 May 2018 16:45:36 -0700
Subject: [PATCH 2/9] move python dict methods to own file

---
 NAMESPACE       |  1 +
 R/python-dict.R | 22 ++++++++++++++++++++++
 R/python.R      | 22 ----------------------
 3 files changed, 23 insertions(+), 22 deletions(-)
 create mode 100644 R/python-dict.R

diff --git a/NAMESPACE b/NAMESPACE
index 75fe0804c..dd4cadc33 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -10,6 +10,7 @@ S3method("<=",python.builtin.object)
 S3method("==",python.builtin.object)
 S3method(">",python.builtin.object)
 S3method(">=",python.builtin.object)
+S3method("[<-",python.builtin.dict)
 S3method("[[",python.builtin.object)
 S3method("[[<-",python.builtin.dict)
 S3method("[[<-",python.builtin.object)
diff --git a/R/python-dict.R b/R/python-dict.R
new file mode 100644
index 000000000..f07892b16
--- /dev/null
+++ b/R/python-dict.R
@@ -0,0 +1,22 @@
+#' @export
+`$<-.python.builtin.dict` <- function(x, name, value) {
+  if (!py_is_null_xptr(x) && py_available())
+    py_dict_set_item(x, name, value)
+  else
+    stop("Unable to assign value (dict reference is NULL)")
+  x
+}
+
+#' @export
+`[<-.python.builtin.dict` <- `$<-.python.builtin.dict`
+
+#' @export
+`[[<-.python.builtin.dict` <- `$<-.python.builtin.dict`
+
+#' @export
+length.python.builtin.dict <- function(x) {
+  if (py_is_null_xptr(x) || !py_available())
+    0L
+  else
+    py_dict_length(x)
+}
diff --git a/R/python.R b/R/python.R
index 68135cd3d..6793fba92 100644
--- a/R/python.R
+++ b/R/python.R
@@ -300,28 +300,6 @@ as.environment.python.builtin.object <- function(x) {
 `[[<-.python.builtin.object` <- `$<-.python.builtin.object`
 
 
-#' @export
-`$<-.python.builtin.dict` <- function(x, name, value) {
-  if (!py_is_null_xptr(x) && py_available())
-    py_dict_set_item(x, name, value)
-  else
-    stop("Unable to assign value (dict reference is NULL)")
-  x
-}
-
-#' @export
-`[[<-.python.builtin.dict` <- `$<-.python.builtin.dict`
-
-#' @export
-length.python.builtin.dict <- function(x) {
-  if (py_is_null_xptr(x) || !py_available())
-    0L
-  else
-    py_dict_length(x)
-}
-
-
-
 #' @export
 .DollarNames.python.builtin.module <- function(x, pattern = "") {
 

From 0fa2aa768c4ee453fad760e43217c873985d55e3 Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Wed, 9 May 2018 16:58:30 -0700
Subject: [PATCH 3/9] give dictionaries their own accessor methods

---
 NAMESPACE       |  3 +++
 R/python-dict.R | 13 +++++++++++++
 R/python.R      |  5 +----
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/NAMESPACE b/NAMESPACE
index dd4cadc33..f63174e30 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -1,6 +1,7 @@
 # Generated by roxygen2: do not edit by hand
 
 S3method("!=",python.builtin.object)
+S3method("$",python.builtin.dict)
 S3method("$",python.builtin.module)
 S3method("$",python.builtin.object)
 S3method("$<-",python.builtin.dict)
@@ -10,7 +11,9 @@ S3method("<=",python.builtin.object)
 S3method("==",python.builtin.object)
 S3method(">",python.builtin.object)
 S3method(">=",python.builtin.object)
+S3method("[",python.builtin.dict)
 S3method("[<-",python.builtin.dict)
+S3method("[[",python.builtin.dict)
 S3method("[[",python.builtin.object)
 S3method("[[<-",python.builtin.dict)
 S3method("[[<-",python.builtin.object)
diff --git a/R/python-dict.R b/R/python-dict.R
index f07892b16..8721852d7 100644
--- a/R/python-dict.R
+++ b/R/python-dict.R
@@ -1,3 +1,16 @@
+#' @export
+`$.python.builtin.dict` <- function(x, name) {
+  if (py_is_null_xptr(x) || !py_available())
+    return(NULL)
+  py_dict_get_item(x, name)
+}
+
+#' @export
+`[.python.builtin.dict` <- `$.python.builtin.dict`
+
+#' @export
+`[[.python.builtin.dict` <- `$.python.builtin.dict`
+
 #' @export
 `$<-.python.builtin.dict` <- function(x, name, value) {
   if (!py_is_null_xptr(x) && py_available())
diff --git a/R/python.R b/R/python.R
index 6793fba92..693707a4c 100644
--- a/R/python.R
+++ b/R/python.R
@@ -243,10 +243,7 @@ py_has_convert <- function(x) {
   }
 
   # get the attrib
-  if (inherits(x, "python.builtin.dict"))
-    attrib <- py_dict_get_item(x, name)
-  else
-    attrib <- py_get_attr(x, name)
+  attrib <- py_get_attr(x, name)
 
   # convert
   if (convert || py_is_callable(attrib)) {

From 0a7538b96182625c5a7102ce0d5a5eed33cd73e4 Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Wed, 9 May 2018 17:09:43 -0700
Subject: [PATCH 4/9] refactor out py_maybe_convert helper

---
 R/python-dict.R |  4 +++-
 R/python.R      | 37 ++++++++++++++++++++-----------------
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/R/python-dict.R b/R/python-dict.R
index 8721852d7..56cfc3efc 100644
--- a/R/python-dict.R
+++ b/R/python-dict.R
@@ -2,7 +2,9 @@
 `$.python.builtin.dict` <- function(x, name) {
   if (py_is_null_xptr(x) || !py_available())
     return(NULL)
-  py_dict_get_item(x, name)
+
+  item <- py_dict_get_item(x, name)
+  py_maybe_convert(item, py_has_convert(x))
 }
 
 #' @export
diff --git a/R/python.R b/R/python.R
index 693707a4c..7c331f9e2 100644
--- a/R/python.R
+++ b/R/python.R
@@ -220,6 +220,24 @@ py_has_convert <- function(x) {
     TRUE
 }
 
+py_maybe_convert <- function(x, convert) {
+  if (convert || py_is_callable(x)) {
+
+    # capture previous convert for attr
+    attrib_convert <- py_has_convert(x)
+
+    # temporarily change convert so we can call py_to_r and get S3 dispatch
+    envir <- as.environment(x)
+    assign("convert", convert, envir = envir)
+    on.exit(assign("convert", attrib_convert, envir = envir), add = TRUE)
+
+    # call py_to_r
+    x <- py_to_r(x)
+  }
+
+  x
+}
+
 #' @export
 `$.python.builtin.object` <- function(x, name) {
 
@@ -242,25 +260,10 @@ py_has_convert <- function(x) {
       return(module)
   }
 
-  # get the attrib
+  # get the attrib and convert as needed
   attrib <- py_get_attr(x, name)
+  py_maybe_convert(attrib, convert)
 
-  # convert
-  if (convert || py_is_callable(attrib)) {
-
-    # capture previous convert for attr
-    attrib_convert <- py_has_convert(attrib)
-
-    # temporarily change convert so we can call py_to_r and get S3 dispatch
-    envir <- as.environment(attrib)
-    assign("convert", convert, envir = envir)
-    on.exit(assign("convert", attrib_convert, envir = envir), add = TRUE)
-
-    # call py_to_r
-    py_to_r(attrib)
-  }
-  else
-    attrib
 }
 
 

From 5b841a4a7d12cc948a5695f902d5cd400047e4a9 Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Wed, 9 May 2018 17:48:01 -0700
Subject: [PATCH 5/9] backwards compatibility for dictionaries

---
 R/python-dict.R | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/R/python-dict.R b/R/python-dict.R
index 56cfc3efc..edd2f8651 100644
--- a/R/python-dict.R
+++ b/R/python-dict.R
@@ -3,15 +3,28 @@
   if (py_is_null_xptr(x) || !py_available())
     return(NULL)
 
+  # for backwards compatibility, `$` prefers items for now,
+  # and falls back to an existing attribute (if any). in
+  # a future release of reticulate, we might consider
+  # having `$` only extract attributes
   item <- py_dict_get_item(x, name)
-  py_maybe_convert(item, py_has_convert(x))
+  if (!py_is_none(item))
+    return(py_maybe_convert(item, py_has_convert(x)))
+
+  `$.python.builtin.object`(x, name)
 }
 
 #' @export
-`[.python.builtin.dict` <- `$.python.builtin.dict`
+`[.python.builtin.dict` <- function(x, name) {
+  if (py_is_null_xptr(x) || !py_available())
+    return(NULL)
+
+  item <- py_dict_get_item(x, name)
+  py_maybe_convert(item, py_has_convert(x))
+}
 
 #' @export
-`[[.python.builtin.dict` <- `$.python.builtin.dict`
+`[[.python.builtin.dict` <- `[.python.builtin.dict`
 
 #' @export
 `$<-.python.builtin.dict` <- function(x, name, value) {

From d140e7ef583eb82d1a3301844bd298a48a011c64 Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Thu, 10 May 2018 10:28:02 -0700
Subject: [PATCH 6/9] define `[[` as `__getitem__`

---
 NAMESPACE  |  1 +
 R/python.R | 36 ++++++++++++++++++++++++------------
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/NAMESPACE b/NAMESPACE
index f63174e30..b70d25395 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -12,6 +12,7 @@ S3method("==",python.builtin.object)
 S3method(">",python.builtin.object)
 S3method(">=",python.builtin.object)
 S3method("[",python.builtin.dict)
+S3method("[",python.builtin.object)
 S3method("[<-",python.builtin.dict)
 S3method("[[",python.builtin.dict)
 S3method("[[",python.builtin.object)
diff --git a/R/python.R b/R/python.R
index 7c331f9e2..c68dfe66f 100644
--- a/R/python.R
+++ b/R/python.R
@@ -238,9 +238,10 @@ py_maybe_convert <- function(x, convert) {
   x
 }
 
-#' @export
-`$.python.builtin.object` <- function(x, name) {
-
+# helper function for accessing attributes or items from a
+# Python object, after validating that we do indeed have
+# a valid Python object reference
+py_get_with <- function(accessor, x, name) {
   # resolve module proxies
   if (py_is_module_proxy(x))
     py_resolve_module_proxy(x)
@@ -249,21 +250,32 @@ py_maybe_convert <- function(x, convert) {
   if (py_is_null_xptr(x) || !py_available())
     return(NULL)
 
-  # deterimine whether this object converts to python
-  convert <- py_has_convert(x)
-
   # special handling for embedded modules (which don't always show
   # up as "attributes")
   if (py_is_module(x) && !py_has_attr(x, name)) {
-    module <- py_get_submodule(x, name, convert)
+    module <- py_get_submodule(x, name, py_has_convert(x))
     if (!is.null(module))
       return(module)
   }
 
   # get the attrib and convert as needed
-  attrib <- py_get_attr(x, name)
-  py_maybe_convert(attrib, convert)
+  object <- accessor(x, name)
+  py_maybe_convert(object, py_has_convert(x))
+}
 
+#' @export
+`$.python.builtin.object` <- function(x, name) {
+  py_get_with(py_get_attr, x, name)
+}
+
+#' @export
+`[.python.builtin.object` <- function(x, name) {
+  py_get_with(py_get_item, x, name)
+}
+
+#' @export
+`[[.python.builtin.object` <- function(x, name) {
+  py_get_with(py_get_item, x, name)
 }
 
 
@@ -815,7 +827,7 @@ py_get_item <- function(x, name, silent = FALSE) {
     py_resolve_module_proxy(x)
 
   if (!py_has_attr(x, "__getitem__"))
-    stop("Python object has no '__getitem__' method")
+    stop("Python object has no '__getitem__' method", call. = FALSE)
   getitem <- py_to_r(py_get_attr(x, "__getitem__", silent = FALSE))
 
   item <- if (silent)
@@ -847,7 +859,7 @@ py_set_item <- function(x, name, value) {
     py_resolve_module_proxy(x)
 
   if (!py_has_attr(x, "__setitem__"))
-    stop("Python object has no '__setitem__' method")
+    stop("Python object has no '__setitem__' method", call. = FALSE)
   setitem <- py_to_r(py_get_attr(x, "__setitem__", silent = FALSE))
 
   setitem(name, value)
@@ -872,7 +884,7 @@ py_del_item <- function(x, name) {
     py_resolve_module_proxy(x)
 
   if (!py_has_attr(x, "__delitem__"))
-    stop("Python object has no '__delitem__' method")
+    stop("Python object has no '__delitem__' method", call. = FALSE)
   delitem <- py_to_r(py_get_attr(x, "__delitem__", silent = FALSE))
 
   delitem(name)

From 692aff36836b10691a0484f6bc2ae3808459d00e Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Thu, 10 May 2018 12:43:53 -0700
Subject: [PATCH 7/9] '$' access attributes then items; `[` opposite

---
 R/python-dict.R | 11 ++++-------
 R/python.R      | 33 ++++++++++++++++++++++++++++-----
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/R/python-dict.R b/R/python-dict.R
index edd2f8651..f9fede0e5 100644
--- a/R/python-dict.R
+++ b/R/python-dict.R
@@ -3,15 +3,12 @@
   if (py_is_null_xptr(x) || !py_available())
     return(NULL)
 
-  # for backwards compatibility, `$` prefers items for now,
-  # and falls back to an existing attribute (if any). in
-  # a future release of reticulate, we might consider
-  # having `$` only extract attributes
-  item <- py_dict_get_item(x, name)
-  if (!py_is_none(item))
+  if (py_has_attr(x, name)) {
+    item <- py_get_attr(x, name)
     return(py_maybe_convert(item, py_has_convert(x)))
+  }
 
-  `$.python.builtin.object`(x, name)
+  `[.python.builtin.dict`(x, name)
 }
 
 #' @export
diff --git a/R/python.R b/R/python.R
index c68dfe66f..975e92362 100644
--- a/R/python.R
+++ b/R/python.R
@@ -241,7 +241,7 @@ py_maybe_convert <- function(x, convert) {
 # helper function for accessing attributes or items from a
 # Python object, after validating that we do indeed have
 # a valid Python object reference
-py_get_with <- function(accessor, x, name) {
+py_get_item_or_attr <- function(x, name, prefer_attr) {
   # resolve module proxies
   if (py_is_module_proxy(x))
     py_resolve_module_proxy(x)
@@ -259,23 +259,46 @@ py_get_with <- function(accessor, x, name) {
   }
 
   # get the attrib and convert as needed
-  object <- accessor(x, name)
+  if (prefer_attr) {
+
+    if (py_has_attr(x, name)) {
+      object <- py_get_attr(x, name)
+    } else {
+      object <- py_get_item(x, name)
+    }
+
+  } else {
+
+    # if we have an attribute, attempt to get the item
+    # but allow for fallback to that attribute
+    if (py_has_attr(x, name)) {
+      object <- py_get_item(x, name, silent = TRUE)
+      if (is.null(object))
+        object <- py_get_attr(x, name)
+    } else {
+      # we don't have an attribute; only attempt item
+      # access and allow normal error propagation
+      object <- py_get_item(x, name)
+    }
+
+  }
+
   py_maybe_convert(object, py_has_convert(x))
 }
 
 #' @export
 `$.python.builtin.object` <- function(x, name) {
-  py_get_with(py_get_attr, x, name)
+  py_get_item_or_attr(x, name, TRUE)
 }
 
 #' @export
 `[.python.builtin.object` <- function(x, name) {
-  py_get_with(py_get_item, x, name)
+  py_get_item_or_attr(x, name, FALSE)
 }
 
 #' @export
 `[[.python.builtin.object` <- function(x, name) {
-  py_get_with(py_get_item, x, name)
+  py_get_item_or_attr(x, name, FALSE)
 }
 
 

From 7f62016df6ef43f6658bd6ae8cd0d22ae5c1cc75 Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Thu, 10 May 2018 12:52:15 -0700
Subject: [PATCH 8/9] unit test

---
 tests/testthat/test-python-dict.R | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/testthat/test-python-dict.R b/tests/testthat/test-python-dict.R
index 1a65dc368..6a39df352 100644
--- a/tests/testthat/test-python-dict.R
+++ b/tests/testthat/test-python-dict.R
@@ -41,3 +41,16 @@ test_that("Dictionary items can be get / set / removed with py_item APIs", {
   expect_error(py_get_item(d, "apple"))
   expect_identical(py_get_item(d, "apple", silent = TRUE), NULL)
 })
+
+test_that("$, [ operators behave as expected", {
+  skip_if_no_python()
+
+  d <- dict(items = 1, apple = 42)
+
+  expect_true(is.function(d$items))
+  expect_true(d['items'] == 1)
+
+  expect_true(d$apple == 42)
+  expect_true(d['apple'] == 42)
+
+})

From e9a4759d77c3011f29f6f0fa7e99c143f24dfe4f Mon Sep 17 00:00:00 2001
From: Kevin Ushey <kevinushey@gmail.com>
Date: Fri, 11 May 2018 12:25:04 -0700
Subject: [PATCH 9/9] use __getitem__ for numeric keys

---
 R/python.R         | 29 ++++++++++++++++++++---------
 man/py_get_item.Rd |  6 +++---
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/R/python.R b/R/python.R
index 975e92362..36e4ba4a8 100644
--- a/R/python.R
+++ b/R/python.R
@@ -241,7 +241,7 @@ py_maybe_convert <- function(x, convert) {
 # helper function for accessing attributes or items from a
 # Python object, after validating that we do indeed have
 # a valid Python object reference
-py_get_item_or_attr <- function(x, name, prefer_attr) {
+py_get_attr_or_item <- function(x, name, prefer_attr) {
   # resolve module proxies
   if (py_is_module_proxy(x))
     py_resolve_module_proxy(x)
@@ -258,6 +258,17 @@ py_get_item_or_attr <- function(x, name, prefer_attr) {
       return(module)
   }
 
+  # re-cast numeric values as integers
+  if (is.numeric(name))
+    name <- as.integer(name)
+
+  # attributes must always be indexed by strings, so if
+  # we receive a non-string 'name', we call py_get_item
+  if (!is.character(name)) {
+    item <- py_get_item(x, name)
+    return(py_maybe_convert(item, py_has_convert(x)))
+  }
+
   # get the attrib and convert as needed
   if (prefer_attr) {
 
@@ -288,17 +299,17 @@ py_get_item_or_attr <- function(x, name, prefer_attr) {
 
 #' @export
 `$.python.builtin.object` <- function(x, name) {
-  py_get_item_or_attr(x, name, TRUE)
+  py_get_attr_or_item(x, name, TRUE)
 }
 
 #' @export
 `[.python.builtin.object` <- function(x, name) {
-  py_get_item_or_attr(x, name, FALSE)
+  py_get_attr_or_item(x, name, FALSE)
 }
 
 #' @export
 `[[.python.builtin.object` <- function(x, name) {
-  py_get_item_or_attr(x, name, FALSE)
+  py_get_attr_or_item(x, name, FALSE)
 }
 
 
@@ -833,18 +844,18 @@ py_list_attributes <- function(x) {
 #'
 #' Retrieve an item from a Python object, similar to how
 #' \code{x[name]} might be used in Python code to access an
-#' item called `name` on an object `x`. The object's
+#' item indexed by `key` on an object `x`. The object's
 #' `__getitem__` method will be called.
 #'
 #' @param x A Python object.
-#' @param name The item name.
+#' @param key The key used for item lookup.
 #' @param silent Boolean; when \code{TRUE}, attempts to access
 #'   missing items will return \code{NULL} rather than
 #'   throw an error.
 #'
 #' @family item-related APIs
 #' @export
-py_get_item <- function(x, name, silent = FALSE) {
+py_get_item <- function(x, key, silent = FALSE) {
   ensure_python_initialized()
   if (py_is_module_proxy(x))
     py_resolve_module_proxy(x)
@@ -854,9 +865,9 @@ py_get_item <- function(x, name, silent = FALSE) {
   getitem <- py_to_r(py_get_attr(x, "__getitem__", silent = FALSE))
 
   item <- if (silent)
-    tryCatch(getitem(name), error = function(e) NULL)
+    tryCatch(getitem(key), error = function(e) NULL)
   else
-    getitem(name)
+    getitem(key)
 
   item
 }
diff --git a/man/py_get_item.Rd b/man/py_get_item.Rd
index 6b5ab329b..f07f57a7a 100644
--- a/man/py_get_item.Rd
+++ b/man/py_get_item.Rd
@@ -4,12 +4,12 @@
 \alias{py_get_item}
 \title{Get an item from a Python object}
 \usage{
-py_get_item(x, name, silent = FALSE)
+py_get_item(x, key, silent = FALSE)
 }
 \arguments{
 \item{x}{A Python object.}
 
-\item{name}{The item name.}
+\item{key}{The key used for item lookup.}
 
 \item{silent}{Boolean; when \code{TRUE}, attempts to access
 missing items will return \code{NULL} rather than
@@ -18,7 +18,7 @@ throw an error.}
 \description{
 Retrieve an item from a Python object, similar to how
 \code{x[name]} might be used in Python code to access an
-item called \code{name} on an object \code{x}. The object's
+item indexed by \code{key} on an object \code{x}. The object's
 \code{__getitem__} method will be called.
 }
 \seealso{