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

Custom filters #70

Merged
merged 11 commits into from
Apr 7, 2023
Merged

Custom filters #70

merged 11 commits into from
Apr 7, 2023

Conversation

passcod
Copy link
Contributor

@passcod passcod commented Apr 7, 2023

Introduces the CustomFilter type and a new method Definitions.insert_custom(name, filter). Custom filters can be constructed from an arity, a run function, and an optional update function. If no update function is provided, the filter cannot be used like filter |= ....

No particular care is taken to make writing filters ergonomic, but it's fairly straightforward nonetheless. For example, here's a basic sqrt:

defs.insert_custom("sqrt", CustomFilter::new(0, |_, (_, val)| {
    Box::new(std::iter::once(match val {
        Val::Float(f) => Ok(Val::Float(f.sqrt())),
        _ => Err(jaq_core::Error::Custom(format!("sqrt: expected float, got {val:?}"))),
    }))
}));

Also:

  • Adds Clone derive (and Debug, Default) to Definitions so it's easier to use in tests to call multiple times.
  • Adds two new errors: NonUpdatable for custom filters that can't be used in update position, and Custom, for use by custom filters.

Fixes #69

@passcod passcod marked this pull request as ready for review April 7, 2023 03:17
@01mf02
Copy link
Owner

01mf02 commented Apr 7, 2023

Wow, thanks for your really fast effort! I'll have a closer look later, but from a first glance, this looks quite good. Well done.

@01mf02 01mf02 merged commit c9c16fb into 01mf02:main Apr 7, 2023
@01mf02
Copy link
Owner

01mf02 commented Apr 7, 2023

I plan to make a few changes to your implementation, in particular replacing the Arc<dyn Fn ...> with just function pointers (fn), and also to implement all named filters (such as take or keys) via your custom filter mechanism.
Thanks again for your really nice idea and your quick implementation, I think that jaq will profit from this greatly. It also came at a perfect time, as I was just trying to figure out a nice way to handle named filters when dealing with recursive definitions. :)

@passcod passcod deleted the native-filters branch April 7, 2023 22:56
@passcod
Copy link
Contributor Author

passcod commented Apr 7, 2023

Awesome, thanks for the quick merge! I look forward to the next release :)

@passcod
Copy link
Contributor Author

passcod commented Apr 8, 2023

One thing I notice using this is that the Filter type given in the custom filter callbacks isn't exposed publicly, which makes it hard to modularise code used in those.

@01mf02
Copy link
Owner

01mf02 commented Apr 8, 2023

One thing I notice using this is that the Filter type given in the custom filter callbacks isn't exposed publicly, which makes it hard to modularise code used in those.

Do you mean the type in jaq_core::filter::Filter? Yes, the API probably has to be exposed more, given that we now have custom filters. I think it would make sense now to expose jaq_core::filter::Filter, which I avoided to do in the past because that would have broken the API every time we added a new core filter. Now, given that we will be able to implement core filters like keys as custom filters, we can expose the filter::Filter type and not break the API every time we add a new core filter.

@passcod
Copy link
Contributor Author

passcod commented Apr 8, 2023

Yeah, that one. Also it should be fine to add #[non_exhaustive] to it so even if new primitive filters are added it won't break the API.

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

Successfully merging this pull request may close these issues.

"Native" filters for jaq-core
2 participants