ignore commented $DATA lines

metrumresearchgroup / bbi

Next generation modeling platform

11 stars 2 forks source link

ignore commented $DATA lines #301

Closed kyleam closed 1 year ago

kyleam commented 1 year ago

As @kylebaron reported in gh-300, bbi nonmem run local gets tripped up by a commented $DATA ... line and tries to hash and record it in bbi_config.json. The last commit of this series fixes that. The second commit fixes a similar issue with bbi nonmem summary ....

The first commit is related to $SIM, not $DATA. ParseRunDetails() now uses the same approach for $DATA and $SIM, which made me look at $SIM and spot that it didn't allow leading whitespace.

Note that, as mentioned at gh-300, the NONMEM help says that the filename for $DATA can be quoted. This series does not add support for that.

seth127 commented 1 year ago

NONMEM help says that the filename for $DATA can be quoted. This series does not add support for that.

I guess I'm fine with not addressing this here. Is it worth making another issue for, so that we can come back to it? I'm trying to get my head around where/how this could bite us...

NONMEM will parse it correctly, so the model will run
We parse this path in the bbi_config.json... would something like this show up as "data_path": ""../../../../extdata/acop.csv"" or "data_path": "'../../../../extdata/acop.csv'"? That seems weird and unlikely.
Would it break our ability to find the file so we can hash it?

I guess my larger question is: have you tried running a control stream with a quoted $DATA path to see what happens? If not, that's probably worth doing at some point, and then potentially opening an issue for it. But I don't think that needs to hold up this PR.

kyleam commented 1 year ago

Is it worth making another issue for, so that we can come back to it?

Yes, I'll create a dedicated issue.

Would it break our ability to find the file so we can hash it?

I haven't tested yet, but, yes, I think it'd fail in the same way that this PR's test case does if you revert the local.go change. bbi splits on spaces, so if the data path is quoted due to spaces, will only get the file up to the first space. Plus, the filename will include a spurious quote at the beginning. So, if quotes are used, hashing will fail when it receives a non-existent file (or, in a very unlikely case, that mangled file will exist, and it will write the hash for a different file).

But I don't think that needs to hold up this PR.

Okay, thanks for your input and review.

kyleam commented 1 year ago

Yes, I'll create a dedicated issue.

gh-302