Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move trim argument of filter to keyword? #2

Open
jariji opened this issue Jun 12, 2023 · 1 comment
Open

Move trim argument of filter to keyword? #2

jariji opened this issue Jun 12, 2023 · 1 comment

Comments

@jariji
Copy link

jariji commented Jun 12, 2023

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)
@Seelengrab
Copy link
Owner

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 Seelengrab changed the title Improve filter trim API Move trim argument of filter to keyword? Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants