update_model_id: use nmrec for updating the table files

[barrettk]

• still uses the old regex. Dont see a way to avoid this
• tests updates. It is now insufficient to just write suffixes to the file, as nmrec must be able to parse individual records
  • added two helper functions in this file for A) setting the default table files, and B) getting the table records

addresses update_model_id() part of https://github.com/metrumresearchgroup/bbr/issues/599

[barrettk]

@kyleam @seth127 let me know what you guys think of the change before you dive into the test edits. I assumed we wanted to remove the need for that regex, but I didn't see a way around it. nmrec can narrow down to the file of a table record, but does not go any deeper into the model id or anything (within the table file name). I didn't see a way to remove the regex, but it now acts on the table records themselves, which is a slight improvement. Apologies if this isn't too clear, but it should make sense when you see the edits

[kyleam]

update_model_id isn't restricted to table records (e.g., the default .suffixes includes .msf and .ext, which would be specified an estimation record), so the proposed changes break update_model_id.

My view is that the old function does what it says it does and is fine as is. nmrec is useful if you want to parse specific records, but, given the documentation for this function pretty much says "replace all occurrences", I think using a regex across the entire control stream is fine.

I didn't see a way to remove the regex, but it now acts on the table records themselves, which is a slight improvement.

If this function was intended to operate on only table records, in my opinion narrowing down the operation the file values of table records (as the proposed changes do) would be a big improvement in terms of the reliability of the operation. The remaining details of changing the value of the file option is left to the caller, and if they decided to change it with a regex, I don't see why that's a problem.

[barrettk]

update_model_id isn't restricted to table records (e.g., the default .suffixes includes .msf and .ext, which would be specified an estimation record), so the proposed changes break update_model_id.

Oh I did not know that. In that case im not sure nmrec makes sense to use here (was hesitant as it was)

My view is that the old function does what it says it does and is fine as is. nmrec is useful if you want to parse specific records, but, given the documentation for this function pretty much says "replace all occurrences", I think using a regex across the entire control stream is fine.

I agree with this 100%

If this function was intended to operate on only table records, in my opinion narrowing down the operation the file values of table records (as the proposed changes do) would be a big improvement in terms of the reliability of the operation. The remaining details of changing the value of the file option is left to the caller, and if they decided to change it with a regex, I don't see why that's a problem.

I dont think it's a problem either. I think the goal of these PRs was to remove as much regex as possible, in favor of using predefined options specified in nmrec, though this case is different from the rest in that we're not operating on a predefined record option (as in the file names themselves).

We could broaden this to work with $EST blocks and wherever else this was intended to make an id replacement, but that will definitely make the logic more complicated - and like you're saying, im not sure that's worth it.

[kyleam]

Sounds like the consensus is to not move to nmrec, and the upcoming fix for gh-611 will introduce conflicts with this, so let's close it. (We can of course resurrect parts of it later, if needed.)