feat: remove inline comments in the preprocessor #74
Conversation
chrispcampbell
changed the title
There was a problem hiding this comment.
Looks good to me. I tested it with the En-ROADS model and verified that the preprocessor produces the same results after your fix as before. I also checked build time of the sde preprocess and sde flatten commands and verified that there was no significant performance hit. (I thought maybe the linear scans would be noticeably slow, but apparently not.)
I will push my additional "check" script for the new comments test to this branch shortly, then will merge it to develop once the build passes.
| @@ -0,0 +1,53 @@ | |||
| {UTF-8} | |||
There was a problem hiding this comment.
I was going to say that it would be nice if we had a way to verify that the mdl output of the preprocessor doesn't contain the comments, but then I remembered that I added such a thing to the test harness a few months ago. I added a check script for the comments test and the output mdl file is as expected, so I'll push that to this branch shortly.
There was a problem hiding this comment.
I pushed that script in b42e664 and verified that the build passes.
| @@ -379,6 +378,34 @@ let matchRegexCaptures = (str, regex) => { | |||
| return [] | |||
| } | |||
| } | |||
| // Match delimiters recursively. Replace delimited strings globally. | |||
| let replaceDelimitedStrings = (str, open, close, newStr) => { | |||
There was a problem hiding this comment.
I think (from inspection and from running the test) that this works as advertised, but (note to selves) at some point we should set up Jest so that we can have more isolated unit tests for these sorts of helper functions. Not a blocking thing; just something that's been on my mind for a long time but haven't had time to work on.
Fixes #73
This PR changes the preprocessor to remove inline comments from the model before parsing it. A test model is included that illustrates several use cases. A special string scanner function was required because a regular expression could not cover all cases. A regexp would need to be greedy to match the case with nested comments, but cannot be greedy to match the case with two comments in one string.