searchlogic icon indicating copy to clipboard operation
searchlogic copied to clipboard

Attribute with multiple parameters

Open ghost opened this issue 15 years ago • 7 comments

Hello, here is a feature request

Using named scope like "created_at_after" with helper like "form.date_select" which produces created_at(1i), created_at(2i) ... are not handled.

@@@ html <% form_for(@foos) do |f| %>

<%= f.label :bar_at, ’Bar at after’ %> <%= f.date_select :bar_at_after %>

<p>
  <%#= f.label :bar_at, &rsquo;Bar at after&rsquo; %>
  <%#= f.text_field :bar_at %>
</p>

<p>
  <%= f.label :bar %>
  <%= f.text_field :bar_equals %>
</p>

<p>
  <%= f.submit :search %>
</p>

@@@

It ends in an obvious attribute(xi) not recognized. I can still used a text_field helper and pass a string as shown in the example, but that’s not always what can be expected in a search form.

It would be nice feature to have.

JH. Chabran

original LH ticket

This ticket has 0 attachment(s).

ghost avatar Aug 07 '09 22:08 ghost

I agree, I tend to always use a javascript calendar date select, etc. So I have never had this issues. I don't think this would be too difficult to solve.

ghost avatar Aug 09 '09 08:08 ghost

I added support for Date, as you can see there : http://github.com/jhchabran/searchlogic/blob/2d3d7ac5d7a267c8a367bf0066683a9212efc1de/lib/searchlogic/search.rb

It lacks support Time class and maybe some cosmetic changes, but it basically works. ( an expectation was added to search_spec too ).

jhchabran avatar Aug 09 '09 22:08 jhchabran

Thanks for doing this. I tried refactoring the code and couldn't really follow it. I can't safely pull that code in because I don't completely understand it and it was really hard to follow.

ghost avatar Aug 10 '09 03:08 ghost

Also, that code would be best in a completely separate module instead of changing the class itself.

ghost avatar Aug 10 '09 03:08 ghost

Separating it in a module is basically doable, but it will leads to hook Search#conditions=, Search#method_missing, which seems to me to be a lot of work for just one line and maybe a bit unreliable if you modify these later.

This feature is clearly not critical, but it feels to me that not supporting all form helpers makes it at just a small step from being completely transparent for developers using it.

You wrote you found it hard to follow, can you detail a bit more ?

Rails multi-parameters are weird and in my opinion should be handled earlier, so params[:created_at] should return a date object or at least one string when submitted with three parameters, which would solve everything in the current case.

This forces me to rewrite handling code for theses parameters because as they did in AR it's coupled with models, and leads to a tricky regexp ( created_at(3i) means third argument with need to be casted as an integer), it's easy to write clear code for that, but in the original AR multi-parameter stuff, they allowed support for things like attribute(4f) and I kept support for these to stay coherent.

It was that part you found confusing or something else ?

And btw while I'm here thanks for SearchLogic, I saved much time using it :-)

jhchabran avatar Aug 12 '09 14:08 jhchabran

I agree, the rails multi parameters are weird and complicated, which is why I try to avoid them. I don't know that your code was hard to follow, its the problem at hand that is hard. My BIG thing with v2 is to keep it simple, and if I add in a feature it needs to be separate out into its own module. v1 got bloated and very hard to maintain. I want to keep v2 simple. Simplicity is better for everyone. Less problems, better performance, etc.

So if you could split this out into a multi parameter module I would add that in. I added your code it and it started to get complicated and harder for me to maintain, which is why I removed it.

Thanks.

ghost avatar Aug 19 '09 05:08 ghost

We needed this for a client project earlier today so I spent some time writing an alternative implementation. It hooks in a bit differently (alias_method_chain with conditions=) but it works well for the stuff I've tested.

It's a bit more complex than jhchabran's approach as well (although, it was inspired / designed like it in some parts) and it's built around AR's implementation / how the AR implementation is coded as of Rails 2.3.

You can see the patch changes at http://github.com/Sutto/searchlogic/, with the actual implementation itself at http://github.com/Sutto/searchlogic/blob/master/lib/searchlogic/search_ext/multiparameter_assignment.rb

Sutto avatar Sep 15 '09 22:09 Sutto