BOSL2 icon indicating copy to clipboard operation
BOSL2 copied to clipboard

Add parameter sanity checking to all functions/modules

Open adrianVmariano opened this issue 4 years ago • 7 comments

Someone mentioned this issue on the forum, and it's a real issue. When you pass the wrong type of parameter to a function/module you often get mysterious errors that are hard to understand.

I think that in any case where it doesn't cripple performance we should add full parameter checking. I was thinking we could have functions like

module assert_list(name,parm) { 
    assert(is_list(parm),
               str("Parameter ",name," must be a list in ",parent_module(1)))
}

Or maybe we make a master type checker function that takes a list of types like ["number","vector"] and allows any one of those types.

Also when parameters are redundant, that should be an error. I've implemented cases like this, but I don't think it's systematic. In other words, if you specify r and d you don't pick one...you give an error.

adrianVmariano avatar Sep 18 '19 22:09 adrianVmariano

Currently, in cyl(), if you specify r and one of r1 or r2, then the value of r will be used for the more specific r1/r2 that wasn't specified. ie: cyl(l=10,r=10,r2=5); is the same as cyl(l=10,r1=10,r2=5);, which is the same as cyl(l=10,r1=10,r=5);

Do we want to forbid this usage and force r1 and r2 to be used together, and assert that r is not used with r1 or r2?

revarbat avatar Sep 19 '19 01:09 revarbat

I think the point is we want to catch user input errors and flag them. I don't want to spend an hour trying to figure out why

cyl(r=1, h=10, d=9);

is producing a 2 unit diameter result no matter how I set d because I just didn't notice that I also passed in r. We also don't want functions/modules to bomb with cryptic backtraces because the code required a list and someone past a scalar.

In the case of r1 and r2 for cyl() I think that d or r (but not both) should be ok if only one of r1 and r2 is defined, but if both r1 and r2 are defined and either of d or r is defined it should be an error. Basically if there is more than one potential interpretation of the input, it should be an error.

adrianVmariano avatar Sep 19 '19 01:09 adrianVmariano

I do think we want to allow mixing r1/d2 or d1/r2

revarbat avatar Sep 19 '19 02:09 revarbat

That's fine. I think that as long as the input is unambiguous, it's fine. But if the input is incomplete or ambiguous then there should be an error.

adrianVmariano avatar Sep 19 '19 02:09 adrianVmariano

I've committed a tweak to get_radius() to make above said checks.

revarbat avatar Sep 19 '19 02:09 revarbat

I noticed that get_radius() is used by a bunch of functions that look like they take different args. Does your tweak work universally?

adrianVmariano avatar Sep 19 '19 03:09 adrianVmariano

I went through all the calls to it, and I'm pretty certain it'll work as expected.

revarbat avatar Sep 19 '19 03:09 revarbat