Closed seth127 closed 9 months ago
Specifically, once the changes from https://github.com/metrumresearchgroup/nmrec/pull/5 are released
This is probably for discussion outside of GitHub, but, in the case of bbr (at least for non-optional code), won't a bbr release that depends on nmrec need to wait for nmrec make it to MPN?
And thanks for pulling together these examples.
won't a bbr release that depends on nmrec need to wait for nmrec make it to MPN?
Hmmm, good thought there. Yes, let's discuss offline. I'm not sure that's a hard requirement, but probably is the smoothest path forward.
I don't think there's anything in-flight here. It probably makes sense to open a dedicated PR for anything else that comes up, but please reopen if I'm missing something.
There are several current
bbr
functions that rely on raw string parsing of the control stream and would likely be more robust if they used the newnmrec
package instead. Specifically, once the changes from https://github.com/metrumresearchgroup/nmrec/pull/5 are released (discussion on release begins here).Several examples:
test_threads()
, notably herenm_table_files()
, specifically herecopy_control_stream()
(internal), specifically here (side note, it's not clear to me that this line needs to be there...)update_model_id()
, really most of the function skipped: see discussion starting at https://github.com/metrumresearchgroup/bbr/pull/605#issuecomment-1660822495These don't need to be done all at once, and some may not be worth doing at all, but they were related enough to put them in the same issue. If anyone sees more cases like this in the code base, feel free to add them to this issue.