inpowell / modulartabler

MIT License
3 stars 0 forks source link

Add support for rangemappingtable to handle non contiguous ranges (#5) #8

Closed inpowell closed 2 months ago

inpowell commented 2 months ago

Solves #5

PeterM74 commented 2 months ago

Non-contiguous update working for me. I submitted a small update as it looks like one of the other updates added a warning to convert_tabular. Annoyingly this warning is one of the timed warnings that only appears once in a time period (as it disappeared when I ran the test again) so I imagine the warning disappeared when you ran the final tests on the PR. Checks are now passing without warnings.

ℹ Testing modulartabler
✔ | F W  S  OK | Context
✔ |          7 | cell_suppression [1.1s]                                                   
✔ |         38 | mapping_table                                                             
✔ |   1      5 | tables_long                                                               
───────────────────────────────────────────────────────────────────────────────────────────
Warning (test-tables_long.R:42:3): convert_tabular converts a semi-long table correctly
Use of .data in tidyselect expressions was deprecated in tidyselect 1.2.0.
ℹ Please use `".valname"` instead of `.data$.valname`
Backtrace:
     ▆
  1. └─modulartabler::convert_tabular(...) at test-tables_long.R:42:2
  2.   ├─dplyr::arrange(...) at modulartabler/R/tables_long.R:119:4
  3.   ├─dplyr::select(...)
  4.   └─dplyr:::select.data.frame(...)
  5.     └─tidyselect::eval_select(expr(c(...)), data = .data, error_call = error_call)
  6.       └─tidyselect:::eval_select_impl(...)
  7.         ├─tidyselect:::with_subscript_errors(...)
  8.         │ └─rlang::try_fetch(...)
  9.         │   └─base::withCallingHandlers(...)
 10.         └─tidyselect:::vars_select_eval(...)
 11.           └─tidyselect:::walk_data_tree(expr, data_mask, context_mask)
 12.             └─tidyselect:::eval_c(expr, data_mask, context_mask)
 13.               └─tidyselect:::reduce_sels(node, data_mask, context_mask, init = init)
 14.                 └─tidyselect:::walk_data_tree(new, data_mask, context_mask)
 15.                   └─tidyselect:::expr_kind(expr, context_mask, error_call)
 16.                     └─tidyselect:::call_kind(expr, context_mask, error_call)
───────────────────────────────────────────────────────────────────────────────────────────

══ Results ════════════════════════════════════════════════════════════════════════════════
Duration: 2.6 s

[ FAIL 0 | WARN 1 | SKIP 0 | PASS 50 ]
inpowell commented 2 months ago

Hmm, unfortunately that change was required to fix an R CMD check NOTE -- this one... looks like neither of our solutions has been perfect.

PeterM74 commented 2 months ago

Hmm, unfortunately that change was required to fix an R CMD check NOTE -- this one... looks like neither of our solutions has been perfect.

Damn, a good reminder to check the check properly! Thanks for catching, happy with this to be merged.