search

openpharma / clinsight

ClinSight - An application for medical monitoring within clinical trials
https://openpharma.github.io/clinsight/
Other
3 stars 0 forks source link

Extending `%|_|%` #112

Open jthompson-arcus opened 2 weeks ago

jthompson-arcus commented 2 weeks ago

The infix function %|_|% added with #104 has an issue if the LHS is an expression instead of an object.

## returns mtcars
mtcars %|_|% iris

## returns iris and throws no warning
head(mtcars) %|_|% iris

Should the function be expanded to handle expressions? Currently the function is fairly lightweight this would add complexity and weight with no clear benefit. Should the function detect the difference between an object and an expression? Should it throw a warning in such a case?

LDSamson commented 2 days ago

My issue with it that, by using it in combination with an expression, it silently returns the value that we are not expecting, which I think is a risk in using this function.

If it is easy to implement, I think ideally it should error (with an informative message) in case an expression is provided. Alternatively, if this is difficult to implement, then we can say that we accept the risk since only core developers will use it, but should be clearly documented with the function that the function should not be used with expressions to protect our future self's from making this mistake.