Refactoring

ElianHugh · Jun 19, 2024 · afa31be · afa31be
1 parent e9e5cee
commit afa31be
Show file tree

Hide file tree

Showing 11 changed files with 84 additions and 39 deletions.
diff --git a/R/cli.R b/R/cli.R
@@ -8,7 +8,7 @@ cli_goodbye <- function() {
 }
 
 cli_watching_directory <- function(engine) {
-    dirs <- c(engine$config$path, engine$config$dirs)
+    dirs <- c(engine$config$entry_dir, engine$config$dirs)
     cli::cli_inform("Watching {.file {dirs}} for changes...")
 }
 

diff --git a/R/config.R b/R/config.R
@@ -9,10 +9,7 @@ new_config <- function(...) {
         "127.0.0.1"
     port <- dots$port %||%
         plumber::get_option_or_env("plumber.port") %||%
-        httpuv::randomPort(
-            host = host,
-            n = 100L
-        )
+        new_port(host = host)
     ignore <- dots$ignore %||%
         utils::glob2rx(
             paste0(
@@ -23,22 +20,24 @@ new_config <- function(...) {
 
     structure(
         list(
-            plumber_path = dots$path,
-            path = dirname(dots$path),
+            entry_path = dots$path,
+            entry_dir = dirname(dots$path),
             dirs = dots$dirs,
             host = host,
             port = port,
-            socket_port = httpuv::randomPort(),
+            socket_port = new_port(
+                used = port,
+                host = host
+            ),
             ignore = ignore
         ),
         class = c("hotwater_config", "list")
-
     )
 }
 
 validate_config <- function(config) {
-    if (!file.exists(config$plumber_path) || dir.exists(config$plumber_path)) {
-        error_invalid_path(config$plumber_path)
+    if (!file.exists(config$entry_path) || dir.exists(config$entry_path)) {
+        error_invalid_path(config$entry_path)
     }
 
     if (!is.null(config$dirs) && any(!dir.exists(config$dirs))) {
@@ -53,4 +52,20 @@ validate_config <- function(config) {
     if (is.numeric(config$host)) {
         error_invalid_host(config$host)
     }
+}
+
+#' it's possible to duplicate the port when it isn't immediately used
+#' this just makes sure we end up with a different number...
+#' @noRd
+new_port <- function(used, host = "127.0.0.1") {
+    out <- NULL
+    if (missing(used)) {
+        out <- httpuv::randomPort(host = host)
+    } else {
+        repeat {
+            out <- httpuv::randomPort(host = host)
+            if (out != used) break
+        }
+    }
+    out
 }
diff --git a/R/engine.R b/R/engine.R
@@ -34,15 +34,15 @@ run_engine <- function(engine) {
 
     current_state <- directory_state(
         c(
-            engine$config$path,
+            engine$config$entry_dir,
             engine$config$dirs
         ),
         engine$config$ignore
     )
 
     repeat {
         poll_runner(engine)
-        current_state <- examine_directory_change(
+        current_state <- watch_directory(
             engine,
             current_state,
             callback
@@ -51,7 +51,10 @@ run_engine <- function(engine) {
 }
 
 kill_engine <- function(engine) {
-    nanonext::reap(engine$publisher)
+    # technically, if we are killing the engine,
+    # we don't want to close the
+    # publlisher because we might reuse it
+    #close(engine$publisher)
     kill_runner(engine)
 }
 

diff --git a/R/run.R b/R/run.R
@@ -39,7 +39,7 @@ run <- function(path, dirs = NULL, port = NULL, host = NULL, ignore = NULL) {
 
 should_reuse_engine <- function(old_config, config) {
     old_exists <- !is.null(old_config)
-    same_path <- identical(old_config$plumber_path, config$plumber_path)
+    same_path <- identical(old_config$entry_path, config$entry_path)
     same_dirs <- identical(old_config$dirs, config$dirs)
     same_port <- identical(old_config$port, config$port) || is.null(config$port)
     same_host <- identical(old_config$host, config$host) || is.null(config$host)

diff --git a/R/runner.R b/R/runner.R
@@ -7,8 +7,6 @@ new_runner <- function(engine) {
                 }
                 plumber::pr(path) |>
                     middleware() |>
-                    # todo, try to leverage plumber defaults,
-                    # likely in the upcoming config
                     plumber::pr_run(
                         port = port,
                         host = host,
@@ -19,10 +17,10 @@ new_runner <- function(engine) {
             error = "error",
             args = list(
                 port = engine$config$port,
-                path = engine$config$plumber_path,
+                path = engine$config$entry_path,
                 host = engine$config$host,
                 middleware = middleware(engine),
-                mod = file.path(getwd(), engine$config$plumber_path)
+                mod = file.path(getwd(), engine$config$entry_path)
             ),
             supervise = TRUE
         )

diff --git a/R/watcher.R b/R/watcher.R
@@ -1,7 +1,6 @@
-# todo, rename this...
-examine_directory_change <- function(engine, current_state, callback) {
+watch_directory <- function(engine, current_state, callback) {
     paths <- c(
-        engine$config$path,
+        engine$config$entry_dir,
         engine$config$dirs
     )
     next_state <- directory_state(paths, engine$config$ignore)

diff --git a/README.md b/README.md
@@ -1,26 +1,26 @@
+<!-- @format -->
+
 # hotwater
 
 <!-- badges: start -->
 
-[![Codecov test
-coverage](https://codecov.io/gh/ElianHugh/hotwater/branch/main/graph/badge.svg)](https://app.codecov.io/gh/ElianHugh/hotwater?branch=main)
-[![R-CMD-check](https://github.com/ElianHugh/hotwater/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/ElianHugh/hotwater/actions/workflows/R-CMD-check.yaml)
 [![Lifecycle: experimental](https://img.shields.io/badge/lifecycle-experimental-orange.svg)](https://lifecycle.r-lib.org/articles/stages.html#experimental)
-<!-- badges: end -->
+[![Codecov test coverage](https://codecov.io/gh/ElianHugh/hotwater/branch/main/graph/badge.svg)](https://app.codecov.io/gh/ElianHugh/hotwater?branch=main)
+[![R-CMD-check](https://github.com/ElianHugh/hotwater/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/ElianHugh/hotwater/actions/workflows/R-CMD-check.yaml)
 
-*work in progress*
+<!-- badges: end -->
 
--   for plumber development
--   autoreload for plumber
--   also auto-refreshes the browser when a change is made
--   run from the commandline with the `/exec/hotwater` bash script
+- for plumber development
+- autoreload for plumber
+- also auto-refreshes the browser when a change is made
+- run from the commandline with the `/exec/hotwater` bash script
 
 ## Installation
 
 You can install the development version of hotwater from
 [GitHub](https://github.com/) with:
 
-``` r
+```r
 # install.packages("devtools")
 devtools::install_github("ElianHugh/hotwater")
 ```
@@ -29,14 +29,14 @@ devtools::install_github("ElianHugh/hotwater")
 
 Hotwater can be run via an R session:
 
-``` r
+```r
 hotwater::run(
     system.file("examples", "plumber.R", package = "hotwater"),
     port = 9999L
 )
 ```
 
-``` r
+```r
 ✔ Server running on <127.0.0.1:9999> [17ms]
 → Watching ./path/to/ for changes...
 ```
@@ -47,7 +47,7 @@ or a terminal using the bash script:
 hotwater -f my/plumber/api.R -p 9999
 ```
 
-``` r
+```r
 ✔ Server running on <127.0.0.1:9999> [17ms]
 → Watching ./path/to/ for changes...
 ```
diff --git a/man/run.Rd b/man/run.Rd
diff --git a/tests/testthat/test-cli.R b/tests/testthat/test-cli.R
@@ -0,0 +1,17 @@
+test_that("startup/teardown messages don't error", {
+    engine <- new_engine(
+        config = new_config(
+            path = system.file("examples", "plumber.R", package = "hotwater")
+        )
+    )
+    expect_no_error(
+        suppressMessages(
+            buildup_engine(engine)
+        )
+    )
+    expect_no_error(
+        suppressMessages(
+            teardown_engine(engine)
+        )
+    )
+})
diff --git a/tests/testthat/test-engine.R b/tests/testthat/test-engine.R
@@ -12,11 +12,24 @@ test_that("engine reuse", {
             old = engine$config,
             new_config(
                 dirs = engine$config$dirs,
-                path = engine$config$plumber_path,
+                path = engine$config$entry_path,
                 port = engine$config$port,
                 host = engine$config$host,
                 ignore = engine$config$ignore
             )
         )
     )
 })
+
+test_that("can kill engine", {
+    engine <- new_engine(
+        config = new_config(
+            path = system.file("examples", "plumber.R", package = "hotwater")
+        )
+    )
+    new_runner(engine)
+    kill_engine(engine)
+
+    expect_false(engine$runner$is_alive())
+    # expect_true(engine$publisher$state == "closed")
+})
diff --git a/tests/testthat/test-middleware.R b/tests/testthat/test-middleware.R
@@ -24,7 +24,7 @@ test_that("middleware injection works with filters", {
     )
     runner <- suppressMessages(callr::r_bg(
         function(config, middleware_filter) {
-            plumber::pr(config$plumber_path) |>
+            plumber::pr(config$entry_path) |>
                 plumber::pr_filter("foo", function(req, res) {
                     stop("I break things")
                 }) |>
@@ -59,7 +59,7 @@ test_that("is_plumber_running works", {
     )
     router <- suppressMessages(callr::r_bg(
         function(config) {
-            plumber::pr(config$plumber_path) |>
+            plumber::pr(config$entry_path) |>
                 plumber::pr_get(
                     "/__hotwater__",
                     function() "running",