Numeric Expression Validation In UcrInput #5826
Conversation
update my master
updating my master
updating my master
update my master
Ucr input
There was a problem hiding this comment.
I tried with the random samples dialogue, and the filter and then when choosing a probability distribution. I tried also making mistakes - like using ln(5) in a function, instead of log. It then gives a sensible error message.
Others may wish to check too, but I suggest that this is ready for @lloyddewit to check the code and then to merge.
@shadrackkibet there was one issue I found that I suggest you may wish to fix as part of the filter improvements rather than this control. When you make a mistake in a formula in the filter, then it is on the sub-dialogue. When you return to the main filter dialogue it can't (of course show the formula, but OK is still enabled. That then gives an error if you press OK.
|
@shadrackkibet Please could you review this first (it's related to one of your PRs so you're a good candidate)? thanks |
| 'is.numeric(x) returns true if the x expression is a valid one. | ||
| 'So we use it here to check validity of the entry | ||
| vecOutput = frmMain.clsRLink.RunInternalScriptGetOutput("is.numeric(" & strText & ")", bSilent:=True) | ||
| If vecOutput IsNot Nothing AndAlso vecOutput.Length > 0 AndAlso Mid(vecOutput(0), 5).ToUpper = "TRUE" Then |
There was a problem hiding this comment.
Please ignore my previous comment deleted:
I was wondering why it is necessary to check the length vecOutput.Length > 0 here, isn't always greater than 0. Could you please explain why it is needed?
There was a problem hiding this comment.
This is meant to ensure that Mid(vecOutput(0), 5).ToUpper = "TRUE" check will not through a runtime error incase the returned CharacterVector collection is empty. I asked myself the same question "isn't always greater than 0" when implementing this, and I wasn't sure of the answer, so to be safe I did that check (of course the alternative would have been a try catch but I found that to be an overkill).
I'm happy to remove that check if what's returned will never be an empty collection.
There was a problem hiding this comment.
Isn't that case checked by vecOutput IsNot Nothing? which you already have?
There was a problem hiding this comment.
vecOutput IsNot Nothing checks for Nothing or simply NULL. vecOutput.Length > 0 checks for empty Collection. A collection can be not NULL but empty. By empty here I means it has no elements in it. For instance; take a "type of" collection like an array, it can have 0 length meaning empty but not Null, same applies to List.
For clarity, notice I'm checking for Nothing(which is simply NULL), then empty(by checking the length), then if those 2 checks pass, I'm taking the first element (index 0) in the collection in this case vecOutput(0), then I'm stripping away unwanted characters using the VB.NET Mid function, then I'm converting to upper case, after which I'm checking if the result is TRUE. If this check passes then the rest follows...
If I call vecOutput(0) when the collection is empty or rather zero length, then a runtime error will be thrown, so in this case vecOutput IsNot Nothing will pass but vecOutput(0) will be an error (accessing an element in an index position that does not exist).
I'm happy to remove that check if what's returned will never be an empty collection or rather a collection of zero length.
There was a problem hiding this comment.
@shadrackkibet @Patowhiz Thank you for the question and nicely explained response. I am very happy to see the null and empty collection checks here. I would like to keep these checks even if RunInternalScriptGetOutput was expected to always return a non-empty collection. The number of checks is always a question of balance, but in my view, these checks make the code safer and more readable.
|
@rdstern You already approved above, please can you 'approve' again so this can be merged? thanks |
Fixes #5762
This PR builds on additions made in PR #5787
@africanmathsinitiative/developers this is ready for review.
I have tested the code changes using regular sequencer and random samples dialog. Please also test with other dialogs.