PropCheck.jl icon indicating copy to clipboard operation
PropCheck.jl copied to clipboard

Move `trim` argument of `filter` to keyword?

Open jariji opened this issue 2 years ago • 1 comments

filter(f, i, trim::Bool) has one of these boolean flag arguments. I don't like these because they're hard to read and dirty up the functional design.

If you feel similarly, some ideas:

  • filter(f, i ; trim::Bool) (easier to read, at least)
  • filter(f, i) and trimfilter(f, i) (I tend to like this from a functional multiple dispatch design perspective)
  • filter(f, i, TRIM) (an enum)

jariji avatar Jun 12 '23 07:06 jariji

I originally used filter(f, i; trim) (i.e., a keyword), but back when I wrote this filtering on the (so far undocumented, but that should be done at some point) Tree object I noticed quite a lot of performance loss in generation & shrinking. I suspect that was the case due to missing constant propagation/some type instability that fixed itself by moving to a positional argument, but I'd have to test that again. It's been a while since I wrote that code.

It should be pretty simple to test out though - change the functions here and here, as well as their uses in the package to use keywords and run the testsuite, comparing before & after. If the runtime changes substantially, it's likely better to stick to the current design (albeit it being a bit less purely functional).

Seelengrab avatar Jun 12 '23 07:06 Seelengrab