Remove `check_lst_file()`

[seth127]

This branch began as a proposed one-liner fix to the changes made in #527, but it expanded a bit in scope as I poked around in the code.

Original purpose

That proposed fix is in the first commit (7f51a88072dd92d9b5742e66b7ac97d6b00cb26b) and I still think we should, at the very least, bring that in as part of #527. More detail is in that commit message.

Refactoring to remove check_lst_file()

The rest of the commits are related to removing check_lst_file() altogether. The basic principle here is that I think we should move towards passing the user request through to bbi and passing back the (ideally reasonable) error that bbi would possibly return. This is a bit wrapped up in another comment I made on the bbi repo. I'm not sure we need to address those together, but I would kind of prefer it.

A bit more on the check_lst_file() function and why I think it's time has come to bid adieau:

• It is quite old and uses some unfortunate patterns (discussed below).
• The original comment above its (only) use in the code says "the output always should be runname.lst", which is true (since bbi will be naming the .lst file) and also proves the point that the glob syntax was never needed in the first place.
• In fact, the aforementioned glob syntax actually created some issues that we had to deal with, and with the changes here we can remove a test that was necessary from a code-path point of view, but testing a situation that would probably never happen in the wild anyway.
  • Although... that commit message does say this was discovered "in the wild", which I'm confused by. Either way, the new behavior would be to always only look for the .lst file at build_path_from_model(.mod, ".lst") (i.e. <run>/<run>.lst), which I think is always the correct thing to do, even in the weird case where there are multiple .lst files in a single output dir.
• As a final note, the commit that check_lst_file() originally came from is called wip: dump for seth which is... funny. I mean, maybe it's late in the evening and that's a bit crude. But it is a little funny, and probably (jokes aside) a good indication that its a relic from a previous era of this codebase.

[kyleam]

that commit message does say this was discovered "in the wild", which I'm confused by.

Skimming over this for the time being (since changes on the bbi side need to land first), but, just for reference, I'll send you the internal thread offline.

[seth127]

thanks for the context. For posterity: it was basically that someone had manually put a second .lst file into their output dir, which caused some weird behavior.

I think I still stand by my basic point that:

• bbi names that file so, assuming the model was run by bbr/bbi, we know what it'll be
• if someone puts in a second .lst, I think ignoring it is probably the best move
• if someone manually renames it... I'm fine with dying and telling them "we can't find the lst file at the path we expect"
• even if it wasn't run with bbr/bbi:
  • <run>/<run>.lst seems to be a pretty strong naming convention anyway
  • we don't make any promises that model_summary() can parse a non-bbr run

[seth127]

I got a clean bill of health with alpha.4 (mentioned here) so I'm gonna go ahead and merge this.