check + document role more thoroughly (closes #37)

R/init-pal.R

@@ -48,7 +48,7 @@
     ...,
     .ns = "elmer"
   ) {
-  check_string(role, allow_empty = FALSE)
+  check_role(role)
   if (!role %in% list_pals()) {
     cli::cli_abort(c(
       "No pals with role {.arg {role}} registered.",

R/pal-add-remove.R

@@ -11,7 +11,7 @@
 #' The pals created by those functions will be persistent across sessions.
 #'
 #' @param role A single string giving a descriptor of the pal's functionality.
-# TODO: actually do this once elmer implements
+#' Cand only contain letters and numbers.
 #' @param prompt A single string giving the system prompt. In most cases, this
 #' is a rather long string, containing several newlines.
 # TODO: only add prefix when not supplied one
@@ -31,12 +31,9 @@
     prompt = NULL,
     interface = c("replace", "prefix", "suffix")
 ) {
-  # TODO: need to check that there are no spaces (or things that can't be
-  # included in a variable name)
-  check_string(role, allow_empty = FALSE)
+  check_role(role)
   check_string(prompt)
 
-  # TODO: make this an elmer interpolate or an .md file
   .stash_prompt(prompt, role)
   parse_interface(interface, role)
 

R/prompt.R

@@ -71,7 +71,7 @@
 #' @rdname prompt
 #' @export
 prompt_new <- function(role, interface, contents = NULL) {
-  check_string(role)
+  check_role(role)
   arg_match0(interface, supported_interfaces)
   check_string(contents, allow_null = TRUE)
 

R/utils.R

@@ -31,5 +31,23 @@ list_pals <- function() {
   gsub(".pal_prompt_", "", prompt_names)
 }
 
+# ad-hoc check helpers -------
+check_role <- function(role, call = caller_env()) {
+  check_string(role, allow_empty = FALSE, call = call)
+
+  if (!is_valid_role(role)) {
+    cli::cli_abort(
+      "{.arg role} must be a single string containing only letters and digits.",
+      call = call
+    )
+  }
+
+  invisible(role)
+}
+
 # miscellaneous ----------------------------------------------------------------
 interactive <- NULL
+
+is_valid_role <- function(role) {
+  grepl("^[a-zA-Z0-9]+$", role)
+}

tests/testthat/_snaps/pal-init.md

@@ -49,9 +49,9 @@
 ---
 
     Code
-      .init_pal("beep bop boop")
+      .init_pal("beepBopBoop")
     Condition
       Error in `.init_pal()`:
-      ! No pals with role `beep bop boop` registered.
+      ! No pals with role `beepBopBoop` registered.
       i See `.pal_add()`.
 

tests/testthat/_snaps/utils.md

@@ -22,3 +22,19 @@
       
       -- A cli pal using gpt-4o-mini. 
 
+# role checks error informatively
+
+    Code
+      check_role("hey there")
+    Condition
+      Error:
+      ! `role` must be a single string containing only letters and digits.
+
+---
+
+    Code
+      check_role(identity)
+    Condition
+      Error:
+      ! `role` must be a single string, not a function.
+

tests/testthat/test-pal-init.R

@@ -26,5 +26,5 @@ test_that("can use other models", {
 test_that("errors informatively with bad role", {
   expect_snapshot(.init_pal(), error = TRUE)
   expect_snapshot(.init_pal(NULL), error = TRUE)
-  expect_snapshot(.init_pal("beep bop boop"), error = TRUE)
+  expect_snapshot(.init_pal("beepBopBoop"), error = TRUE)
 })

tests/testthat/test-utils.R

@@ -11,3 +11,26 @@ test_that(".pal_last is up to date with most recent pal", {
   .init_pal("cli", "chat_openai", model = "gpt-4o-mini")
   expect_snapshot(env_get(pal_env(), ".pal_last"))
 })
+
+
+test_that("role checks error informatively", {
+  expect_snapshot(error = TRUE, check_role("hey there"))
+  expect_snapshot(error = TRUE, check_role(identity))
+})
+
+test_that("is_valid_role works", {
+  expect_true(is_valid_role("role"))
+  expect_true(is_valid_role("newRole"))
+  expect_true(is_valid_role("role123"))
+  expect_true(is_valid_role("ROLE"))
+  expect_true(is_valid_role("r"))
+  expect_true(is_valid_role("1"))
+
+  expect_false(is_valid_role("new role"))
+  expect_false(is_valid_role("new@role"))
+  expect_false(is_valid_role("role_"))
+  expect_false(is_valid_role("role-"))
+  expect_false(is_valid_role("new-role"))
+  expect_false(is_valid_role("role\n123"))
+  expect_false(is_valid_role(" "))
+})