From 062b8bad8af6159d37e87fc55cf43be833900e99 Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Thu, 27 Jun 2024 08:01:04 -0500
Subject: [PATCH] Drop `parse_all()` default method; add tests

---
 NAMESPACE                   |  1 -
 NEWS.md                     |  1 +
 R/parse.R                   | 19 +++++-----------
 tests/testthat/test-parse.R | 43 +++++++++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/NAMESPACE b/NAMESPACE
index 876d889..aea68ec 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -4,7 +4,6 @@ S3method(parse_all,"function")
 S3method(parse_all,call)
 S3method(parse_all,character)
 S3method(parse_all,connection)
-S3method(parse_all,default)
 S3method(print,evaluate_evaluation)
 S3method(replay,character)
 S3method(replay,condition)
diff --git a/NEWS.md b/NEWS.md
index eb636ef..64003fe 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,6 @@
 # evaluate (development version)
 
+* `parse_all()` no longer has a default method, which will generate better errors if you pass in something unexpectected.
 * The package now depends on R 4.0.0 in order to decrease our maintenance burden.
 * `evaluate()` automatically strips calls from conditions emitted by top-level code (these incorrectly get calls because they're wrapped inside `eval()`) (#150).
 * `evalute(include_timing)` has been deprecated. I can't find any use of it on GitHub, and it adds substantial code complexity for little gain.
diff --git a/R/parse.R b/R/parse.R
index fa3eaf2..5e0713e 100644
--- a/R/parse.R
+++ b/R/parse.R
@@ -131,12 +131,12 @@ append_break <- function(x) {
 #' @export
 parse_all.connection <- function(x, filename = NULL, ...) {
   if (!isOpen(x, "r")) {
-      open(x, "r")
-      on.exit(close(x))
+    open(x, "r")
+    defer(close(x))
   }
   text <- readLines(x)
-  if (is.null(filename))
-    filename <- summary(x)$description
+  filename <- filename %||% summary(x)$description
+  
   parse_all(text, filename, ...)
 }
 
@@ -146,19 +146,10 @@ parse_all.function <- function(x, filename = NULL, ...) {
   parse_all(find_function_body(x), filename = filename, ...)
 }
 
-#' @export
-parse_all.default <- function(x, filename = NULL, ...) {
-  if (is.null(filename))
-    filename <- "<expression>"
-  parse_all(deparse(x), filename, ...)
-}
-
 # Calls are already parsed and always length one
 #' @export
 parse_all.call <- function(x, filename = NULL, ...) {
-  out <- parse_all.default(x, filename = filename, ...)
-  out$expr <- list(as.expression(x))
-  out
+  parse_all(deparse(x), filename = filename, ...)
 }
 
 find_function_body <- function(f) {
diff --git a/tests/testthat/test-parse.R b/tests/testthat/test-parse.R
index d8f8926..c41d386 100644
--- a/tests/testthat/test-parse.R
+++ b/tests/testthat/test-parse.R
@@ -30,6 +30,49 @@ test_that("can ignore parse errors", {
   expect_error(evaluate('x <-', stop_on_error = 0), NA)
 })
 
+# input types ------------------------------------------------------------------
+
+test_that("can parse a call", {
+  out <- parse_all(quote(f(a, b, c)))
+  expect_equal(out$src, "f(a, b, c)")
+  expect_equal(
+    out$expr,
+    I(list(expression(f(a, b, c)))),
+    ignore_attr = "srcref"
+  )
+})
+
+test_that("can parse a connection", {
+  path <- withr::local_tempfile(lines = c("# 1", "1 + 1"))
+  cur_cons <- getAllConnections()
+
+  con <- file(path)
+  out <- parse_all(con)
+
+  expect_equal(out$src, c("# 1\n", "1 + 1"))
+  expect_equal(
+    out$expr,
+    I(list(expression(), expression(1 + 1))),
+    ignore_attr = "srcref"
+  )
+
+  # Doesn't leave any connections around
+  expect_equal(getAllConnections(), cur_cons)
+})
+
+test_that("can parse a function", {
+  out <- parse_all(function() {
+    # Hi
+    1 + 1
+  })
+  expect_equal(out$src, c("# Hi\n", "1 + 1"))
+  expect_equal(
+    out$expr,
+    I(list(expression(), expression(1 + 1))),
+    ignore_attr = "srcref"
+  )
+})
+
 # find_function_body -----------------------------------------------------------
 
 test_that("parsing a function parses its body", {