Skip to content

Commit 6175d25

Browse files
Copilotkrlmlr
andcommitted
Simplify Ryaml_yoink: remove factor special-casing, always copyMostAttrib
Remove the factor-specific code path that converted factors to STRSXP in Ryaml_yoink(). Instead, always copy the integer value and call copyMostAttrib() unconditionally. The existing emit_factor() in emit_object() handles factor resolution correctly. This ensures factor handlers work with column.major = FALSE, just like Date and POSIXt handlers. Added tests for factor columns. Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
1 parent 33941e4 commit 6175d25

File tree

2 files changed

+18
-20
lines changed

2 files changed

+18
-20
lines changed

src/r_emit.c

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -261,31 +261,18 @@ static yaml_scalar_style_t Ryaml_string_style(SEXP s_obj)
261261
/* Take a vector and an index and return another vector of size 1 */
262262
static SEXP Ryaml_yoink(SEXP s_vec, int index)
263263
{
264-
SEXP s_tmp = NULL, s_levels = NULL;
265-
int type = 0, factor = 0, level_idx = 0;
264+
SEXP s_tmp = NULL;
265+
int type = 0;
266266

267267
type = TYPEOF(s_vec);
268-
factor = type == INTSXP && Ryaml_has_class(s_vec, "factor");
269-
PROTECT(s_tmp = allocVector(factor ? STRSXP : type, 1));
268+
PROTECT(s_tmp = allocVector(type, 1));
270269

271270
switch(type) {
272271
case LGLSXP:
273272
LOGICAL(s_tmp)[0] = LOGICAL(s_vec)[index];
274273
break;
275274
case INTSXP:
276-
if (factor) {
277-
s_levels = getAttrib(s_vec, R_LevelsSymbol);
278-
level_idx = INTEGER(s_vec)[index];
279-
if (level_idx == NA_INTEGER || level_idx < 1 || level_idx > LENGTH(s_levels)) {
280-
SET_STRING_ELT(s_tmp, 0, NA_STRING);
281-
}
282-
else {
283-
SET_STRING_ELT(s_tmp, 0, STRING_ELT(s_levels, level_idx - 1));
284-
}
285-
}
286-
else {
287-
INTEGER(s_tmp)[0] = INTEGER(s_vec)[index];
288-
}
275+
INTEGER(s_tmp)[0] = INTEGER(s_vec)[index];
289276
break;
290277
case REALSXP:
291278
REAL(s_tmp)[0] = REAL(s_vec)[index];
@@ -300,9 +287,7 @@ static SEXP Ryaml_yoink(SEXP s_vec, int index)
300287
RAW(s_tmp)[0] = RAW(s_vec)[index];
301288
break;
302289
}
303-
if (!factor) {
304-
copyMostAttrib(s_vec, s_tmp);
305-
}
290+
copyMostAttrib(s_vec, s_tmp);
306291
UNPROTECT(1);
307292

308293
return s_tmp;

tests/testthat/test-as_yaml.R

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,3 +503,16 @@ test_that("POSIXct handler works with column.major = FALSE", {
503503
expect_equal(result_col, "time:\n- '2012-10-10'\n- '2014-03-28'\n")
504504
expect_equal(result_row, "- time: '2012-10-10'\n- time: '2014-03-28'\n")
505505
})
506+
507+
test_that("factor column works with column.major = FALSE", {
508+
x <- data.frame(x = factor(c("a", "b", "c")))
509+
result <- as.yaml(x, column.major = FALSE)
510+
expect_equal(result, "- x: a\n- x: b\n- x: c\n")
511+
})
512+
513+
test_that("factor handler works with column.major = FALSE", {
514+
x <- data.frame(x = factor(c("a", "b", "c")))
515+
handler <- list(factor = function(x) paste0("level_", x))
516+
result <- as.yaml(x, handlers = handler, column.major = FALSE)
517+
expect_equal(result, "- x: level_a\n- x: level_b\n- x: level_c\n")
518+
})

0 commit comments

Comments
 (0)