Create dump/1 for saving filters.

[gen1321]

Closes #42. The issue also mentions implementing Ecto Type. If you can provide an example, I will do my best to implement this too.

[rcdilorenzo]

I think this is a great start @gen1321. Here are my comments.

I think we need to more clearly represent the terminology here. While we have parse for understanding data (which makes sense due to the human-like terms in there parameters), dump doesn't quite give enough. I realized I originally used that word in my issue, but I think that encode_map_value will work better since it gives the idea of what form we are encoding to. Additionally, when we implement the custom type, we don't want to overload the term dump since that's used by Ecto.

Because I'm suggesting the addition of a Filtrex.Encoder.Map (see individual comments), I think we should also be consistent and rename Filtrex.Encoder to Filtrex.Encoder.Fragment since it maps to Ecto fragment structures. If you need guidance on creating a protocol, checkout that encoder to get an idea.

As far as a custom type. I think that you would add Filtrex.Ecto.Type as the module name. For implementation details, checkout my article on Medium on how to do this: https://medium.com/@rcdilorenzo/creating-a-custom-ecto-duration-type-994e32ad4613.

[gen1321]

First of all thanks for a great review! It's a bit unclear to me how exactly you want to name/place encoders, my guess is:

1. Create encoders folders.
2. Move lib/encoder.ex to lib/encoders/fragment_encoder.ex folder with Filtrex.Encoders.FragmentEncoder
3. Add llb/encoders/map_encoder with Filtrex.Encoders.MapEncoder inside
4. Rename Filtrex.Utils.Encoder to Filtrex.Utils.FragmentEncoderDSL

[rcdilorenzo]

Yes. That sounds good with one minor tweak on the names. Let's use Filtrex.Encoders.Fragment and Filtrex.Encoders.Map to reduce some verbosity. That means they would go in lib/encoders/fragment.ex and lib/encoders/map.ex respectively.

[gen1321]

About ecto type, how can we pass config to cast function of ecto type? As I can understand we probably need to implement custom casting function like cast_filter(changeset, key, config). Anyway, maybe it's a good idea to separate this into another PR?

[rcdilorenzo]

@gen1321 Ah, I hadn't thought about that. 🤔 I think we'll probably have to have a module that allows you to define your own type that uses a hardcoded configuration (or calls a module). For example, the helper module might be Filtrex.Ecto.Type but a custom module would be MyApp.Filtrex.UserType. Regardless, I agree that it should be a separate pull request.

[gen1321]

Is there something left? looks done to me

[rcdilorenzo]

So sorry that I haven't circled back on this. Last semester of a graduate degree will do that to a person... 😄 These are the final changes I think are necessary. I'll try to be quicker to respond as you make changes. Thanks for sticking it out!

[rcdilorenzo]

Because filtrex elsewhere allows for an arbitrary level of recursion for sub_filters, let's make sure this is consistent with this encoding feature being added. Checkout schema.json for the definition.

[gen1321]

@rcdilorenzo done

[rcdilorenzo]

Have you actually seen the recursion working properly? It looks like the code is right, but it might be worth it to throw in an example where we have a sub-filter that has to be dumped. (I'll do a final review to make sure we didn't miss anything, but everything in the new commits look good.)