Closed kyleam closed 2 years ago
Marked as draft until I figure out the CI failures (devtools::check()
passed for me on metworx with alpha.3).
script/boot-collect.R
Thanks for testing this with a larger case.
[weird result 1] I can follow the code path and see why this happens and I guess it's fine. But in my thinking (and the previous behavior) I'm pretty sure this was only TRUE when the model initially errored, but then didn't error with the .fail_flags.
Thanks. I missed that detail in my conversion. I think it'd be good for both code paths to be consistent (even if it's unlikely to matter in practice).
[weird result 2] the 005 model, where I overwrote the .lst with only the text "this will break", actually parses when no_ext_file = T is passed. I noticed this in summary_log(), but I can reproduce it with a single model_summary() call too. Not a big deal, but this should... probably fail.
Hmm, yeah. I'll take a closer look.
[weird result 1]
This one should be resolved with 06562409 (model_summaries_concurrent: fix .fail_flags discrepancy, 2022-08-02).
While working on that, I realized that gh-529 is too aggressive because it leads to some JSON parsing failures with older versions of bbi. I've added back check_lst_file
(220a931b), calling it only for bbi versions before 3.2.0. @seth127, let me know if you have any objections to that.
remaining todos:
inst/test-refs/1_summary_obj.R
and (at least at the moment) conceptual conflicts@seth127: there have been some substantial changes since you last took a look (c36155b6d), especially reintroducing check_lst_file
, so I've re-requested your review.
$ git log --oneline --first-parent --no-decorate c36155b6d..
8a44798e Merge branch 'main' into summary-multi
ce785a40 model_summaries_concurrent: replace a subsetted map with modify_at
06562409 model_summaries_concurrent: fix .fail_flags discrepancy
bc74814d model_summaries: handle NULL .fail_flags
220a931b model-summary: restore check_lst_file() check for older bbi versions
2073187c test-refs/1_summary_obj.R: regenerate with build-model-summary-refs.R
@seth127 Thanks for taking another look.
It's a bit unfortunate that we need that convoluted code in there to support the pre-3.2.0 behavior, but oh well
Agreed. I looked forward to whenever we bump the min bbi version and can strip out that and similar compatibility kludges.
This series addresses gh-395 and builds on top of changes on the bbi side (https://github.com/metrumresearchgroup/bbi/pull/272) that are available under the
v3.2.0-alpha.3
dev release.Commit overview:
Closes #395.