fixes #5577 Errors in R-Instat repeat the previous command

[lloyddewit]

Fixes #5577. This was caused by the following:

• RLink.RunScript(...), line 570
  • Calls Evaluate(...) with the R script (e.g. .temp_val <- capture.output(2+x))
  • Evaluate(...) returns True if script executed successfully, else False
  • Line 570 ignores the return value
• RLink.RunScript(...), lines 571-577
  • Extracts and displays the value of .temp_val
  • If the script was not successful then .temp_val still contains its previous value (or is potentially undefined)

Issue fixed by:

• line 570: Check the return value of Evaluate(...)
• If True then
  • Extract and display the value of .temp_val
  • Remove .temp_val (as recommended by @dannyparsons it's good to keep the R environment tidy and remove variables after using them)
• Else do nothing

[dannyparsons]

This looks correct. @rdstern could you test with a few examples?

[dannyparsons]

You can remove = True, it will do the same without it.

[lloyddewit]

thank you, done

[dannyparsons]

@lloyddewit I have pushed changes to this branch to address Roger's issue here: #5577 (comment). Please check and merge if it looks correct.

The reason this was not trapped by the original fix is that the script is slightly different:

Last_Test <- cor.test( size,fert2)
data_book$add_model(model_name="Last_Test", model=Last_Test, data_name="survey")
data_book$get_models(data_name="survey", model_name="Last_Test")

In RunScript() the last line is separated, so that we only capture output from the last line. But it's the first of the three lines that gives the error. In my edit I have added a boolean to store whether the first part of the script gives an error. If it does, then it doesn't attempt to run the last line and display any output.

The reason the last line doesn't give an error itself is that when Roger runs the dialog the first time with a correct command then it does save something called "Last_Test" into the data_book, hence the line:

data_book$get_models(data_name="survey", model_name="Last_Test")

does run, but its getting the model from the first successful run of the dialog (because the name is the same).

I hope that's clear.

[lloyddewit]

@dannyparsons thanks for the changes and explanation. I approved and merged.
In your example above, is it practical to delete Last_Test after the second line?

Note: RunScript seems like a core function. is it a candidate for refactoring or adding extra technical documentation?

[dannyparsons]

Great. I wouldn't delete Last_Test in this case, since it still holds the last successfully run output from the dialog. But there could be a bit of confusion for a user to know what's happened when they get an error.

Yes definitely RunScript() is a core function and it would be benefit to new developers to have some technical documentation.