From c87bee535e52d2fdb784895cf87df349d6b3d4b0 Mon Sep 17 00:00:00 2001 From: Mossa Date: Fri, 31 Jul 2026 16:03:10 +0200 Subject: [PATCH] Write nested lists correctly (#5) `as_item()` decided between an array and a table based on whether the *key* was named, not whether the value list itself was named. Any unnamed list passed as a value therefore took the table branch, where `as_kv_pairs()` inserted every element under the literal key `NA`, keeping only the last one. Derive it from the value's own names instead, mirroring `as_value()`, and turn a list of named lists into an array of tables so an item read with `get_item()` can be written back with `insert_items()`. Co-Authored-By: Claude Opus 5 (1M context) --- NEWS.md | 8 +++ src/rust/src/item.rs | 24 +++++--- src/rust/src/lib.rs | 2 +- src/rust/src/table.rs | 39 ++++++++++-- tests/testthat/test-nested-lists.R | 96 ++++++++++++++++++++++++++++++ 5 files changed, 156 insertions(+), 13 deletions(-) create mode 100644 tests/testthat/test-nested-lists.R diff --git a/NEWS.md b/NEWS.md index f2b0ddc..08dd517 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,14 @@ * extend capabilities of writing and printing to support formatting. +* Unnamed lists are now written as arrays wherever they appear. Previously an + unnamed list passed as the value of a key was written as a table with a single + `NA` key, keeping only the last element (#5). + +* A list of named lists is now written as an array of tables, so an item read + with `get_item()` can be inserted back into a document with `insert_items()` + (#5). + # tomledit 0.1.1 * Unnamed lists are now turned into an array of items. Any named listed are treated as inline tables. diff --git a/src/rust/src/item.rs b/src/rust/src/item.rs index 457ba4e..ac12c33 100644 --- a/src/rust/src/item.rs +++ b/src/rust/src/item.rs @@ -1,10 +1,12 @@ -use crate::{as_array_list, as_array_of_tables, as_table, as_value, TomlEditRError}; +use crate::{ + as_array_list, as_array_of_tables, as_table, as_tables, as_value, is_table_like, TomlEditRError, +}; use extendr_api::prelude::*; use std::result::Result as Res; use toml_edit::Item; // Item wrapper -pub(crate) fn as_item(x: Robj, named: bool, df_as_array: bool) -> Res { +pub(crate) fn as_item(x: Robj, df_as_array: bool) -> Res { match x.rtype() { Rtype::Null => Ok(Item::None), Rtype::Rstr | Rtype::Logicals | Rtype::Integers | Rtype::Strings | Rtype::Doubles => { @@ -16,13 +18,19 @@ pub(crate) fn as_item(x: Robj, named: bool, df_as_array: bool) -> Res Ok(Item::ArrayOfTables(as_array_of_tables(List::try_from(x)?)?)), false => Ok(Item::Table(as_table(List::try_from(x)?)?)), } - } else if !named { - let mut xx = as_array_list(x.try_into()?)?; - xx.decor_mut().set_prefix("\n"); - - Ok(Item::Value(toml_edit::Value::Array(xx))) - } else { + } else if x.names().is_some() { + // named lists are tables, just like in `as_value()` Ok(Item::Table(as_table(List::try_from(x)?)?)) + } else { + let xx = List::try_from(x)?; + + // a list of named lists has no array representation that keeps + // the elements distinct, so it becomes an array of tables + if !xx.is_empty() && (0..xx.len()).all(|i| is_table_like(&xx[i])) { + return Ok(Item::ArrayOfTables(as_tables(xx)?)); + } + + Ok(Item::Value(toml_edit::Value::Array(as_array_list(xx)?))) } } _ => Err(TomlEditRError::CrateError(format!( diff --git a/src/rust/src/lib.rs b/src/rust/src/lib.rs index a431e93..09588bf 100644 --- a/src/rust/src/lib.rs +++ b/src/rust/src/lib.rs @@ -76,7 +76,7 @@ impl Toml { let mut new = self.clone(); for (k, v) in x.into_iter() { - new.0.insert(k, as_item(v, !k.is_empty(), df_as_array)?); + new.0.insert(k, as_item(v, df_as_array)?); } Ok(new) diff --git a/src/rust/src/table.rs b/src/rust/src/table.rs index 841be0a..46b5100 100644 --- a/src/rust/src/table.rs +++ b/src/rust/src/table.rs @@ -3,16 +3,30 @@ use extendr_api::prelude::*; use std::result::Result as Res; use toml_edit::{ArrayOfTables, InlineTable, Item, Table, Value}; +// Is this something that becomes a table, i.e. a named list? +pub(crate) fn is_table_like(x: &Robj) -> bool { + x.rtype() == Rtype::List && !x.inherits("data.frame") && x.names().is_some() +} + // Tables pub(crate) fn as_kv_pairs(x: List) -> Res, TomlEditRError> { let err = Err(TomlEditRError::CrateError(String::from( "Lists must contain only named elements or no named elements", ))); - let names = x.names(); - if let Some(mut nm) = names { - if nm.any(|xi| xi.is_empty()) { - return err; + match x.names() { + Some(mut nm) => { + if nm.any(|xi| xi.is_empty()) { + return err; + } + } + // without names there are no keys to insert the values under + None => { + if !x.is_empty() { + return Err(TomlEditRError::CrateError(String::from( + "A table cannot be created from an unnamed list", + ))); + } } } let kvs = x @@ -39,6 +53,23 @@ pub(crate) fn as_table(x: List) -> Res { Ok(Table::from_iter(kvs)) } +// Array of Tables from a list of named lists e.g. `[[packages]]` entries +pub(crate) fn as_tables(x: List) -> Res { + let mut arr = ArrayOfTables::new(); + + for i in 0..x.len() { + let tbl = x[i].clone(); + if !is_table_like(&tbl) { + return Err(TomlEditRError::CrateError(String::from( + "An array of tables can only be created from named lists", + ))); + } + arr.push(as_table(List::try_from(tbl)?)?); + } + + Ok(arr) +} + // Array of Tables pub(crate) fn as_array_of_tables(x: List) -> Res { if !x.inherits("data.frame") { diff --git a/tests/testthat/test-nested-lists.R b/tests/testthat/test-nested-lists.R new file mode 100644 index 0000000..67c4c55 --- /dev/null +++ b/tests/testthat/test-nested-lists.R @@ -0,0 +1,96 @@ +# An `rv.lock` (version 2) style document, from +# https://github.com/extendr/tomledit/issues/5 +lockfile <- '# This file is automatically @generated by rv. +version = 2 +r_version = "4.4" + +[[packages]] +name = "DT" +version = "0.33" +source = { repository = "https://example.com/rpkgs/2025-04-26" } +force_source = false +dependencies = [ + { name = "htmltools", requirement = "(>= 0.3.6)" }, + "httpuv", + { name = "jsonlite", requirement = "(>= 0.9.16)" }, + "magrittr", +] + +[[packages]] +name = "Matrix" +version = "1.7-1" +source = { builtin = true } +force_source = false +dependencies = [ + "lattice", + "grid", +] +' + +test_that("arrays containing inline tables are read as lists", { + pkgs <- get_item(parse_toml(lockfile), "packages") + + expect_length(pkgs, 2) + expect_equal(pkgs[[1]]$name, "DT") + expect_equal(pkgs[[1]]$source, list(repository = "https://example.com/rpkgs/2025-04-26")) + + deps <- pkgs[[1]]$dependencies + expect_length(deps, 4) + expect_equal(deps[[1]], list(name = "htmltools", requirement = "(>= 0.3.6)")) + expect_equal(deps[[2]], "httpuv") +}) + +test_that("unnamed lists are written as arrays", { + expect_equal( + to_toml(as_toml(list(x = list(1, 2, 3)))), + "x = [1.0, 2.0, 3.0]\n" + ) + + expect_equal( + to_toml(as_toml(list(deps = list("a", list(name = "b", requirement = ">=1"))))), + "deps = [\"a\", { name = \"b\", requirement = \">=1\" }]\n" + ) + + expect_equal(to_toml(as_toml(list(x = list()))), "x = []\n") +}) + +test_that("a list of named lists is written as an array of tables", { + expect_equal( + to_toml(as_toml(list(x = list(list(a = 1L), list(a = 2L))))), + "[[x]]\na = 1\n\n[[x]]\na = 2\n" + ) +}) + +test_that("an array of tables survives a round trip", { + doc <- parse_toml(lockfile) + pkgs <- get_item(doc, "packages") + + round_tripped <- insert_items(parse_toml(lockfile), packages = pkgs) + expect_equal( + get_item(parse_toml(to_toml(round_tripped)), "packages"), + pkgs + ) + + # the rest of the document is left alone + expect_equal(get_item(round_tripped, "version"), 2L) + expect_true(any(grepl("@generated by rv", as.character(round_tripped)))) +}) + +test_that("named lists and data.frames are unaffected", { + expect_equal( + to_toml(as_toml(list(x = list(a = 1L, b = 2L)))), + "[x]\na = 1\nb = 2\n" + ) + + df <- data.frame(a = 1:2, b = c("x", "y")) + expect_equal( + to_toml(as_toml(list(x = df))), + "[[x]]\na = 1\nb = \"x\"\n\n[[x]]\na = 2\nb = \"y\"\n" + ) + expect_equal( + to_toml(as_toml(list(x = df), df_as_array = FALSE)), + "[x]\na = [1, 2]\nb = [\"x\", \"y\"]\n" + ) + + expect_error(as_toml(list(x = list(a = 1, 2)))) +})