> But I don't think the > alternative of checking responds_to? is much slower.
yea i think you're right. probably not much benefit for the problems that you mention and the one i mentioned On Apr 14, 5:42 pm, Shawn <[email protected]> wrote: > On Apr 15, 10:09 am, mooman <[email protected]> wrote: > > > > + if cond.responds_to?(:empty?) && cond.empty? > > > + clone > > > + else > > > how about more of an "optimistic" test like: > > > if (cond.empty? rescue false) > > clone > > end > > I considered proposing that, but the (do_something rescue nil/false) > construct has bitten me with unexpected results one than once, so I > now try to avoid it in my own code. In this specific case, if someone > passes in a bad value (due to bugs in their code), or has a custom > class with its own empty? method, then this code will rescue > exceptions on empty? that ought to be raised. > > That said, if Jeremy feels confident about implementing it that way, I > don't really have a problem with it. But I don't think the > alternative of checking responds_to? is much slower. > > Shawn > > > dont know how pretty that is, but should generally be faster than > > checking if the receiver responds to a method then call it, and should > > work the same, right? > > > the only place where it would be much slower is if your cond really > > doesn't respond to empty?, but how likely is it that cond responds to > > "size" (the first line of that method), but not to "empty?". Then > > again, i dont know much of Sequel's inner-workings. > > > On Apr 11, 5:14 pm, Shawn <[email protected]> wrote: > > > > Hi Jeremy, > > > > Yes, I've used the ds = ds.filter approach as a workaround. However, > > > if there are a number of attributes being added conditionally, then > > > the repeated dataset filters are less efficient than defining a hash > > > of attributes and calling filter once. > > > > I believe your patch could be made more efficient. You're using > > > Array.include to look for empty condition expressions, when the empty? > > > method should be a lot faster: > > > > + if cond.responds_to?(:empty?) && cond.empty? > > > + clone > > > + else > > > > This would not match cond == [[]], but I don't know that filter should > > > honor that condition expression. [], {}, and '' would all be empty > > > and result in a clone. > > > > A more universally applicable way to implement this would be to fork > > > inside the filter_expr method: > > > > if expr?(:empty?) && expr.empty? > > > return BOOL_TRUE # not sure how you get the true value for the > > > dataset's database, but insert it here > > > end > > > > Then a fork would not be necessary inside Dataset.filter: > > > > cond = filter_expr(cond, &block) #=> BOOL_TRUE > > > cond = SQL::BooleanExpression.new(:AND, @opts[clause], cond) if > > > @opts[clause] > > > clone(clause => cond) #=> where TRUE (1, 't', etc.) > > > > Shawn > > > > On Apr 10, 7:16 am, Jeremy Evans <[email protected]> wrote: > > > > > On Apr 9, 1:57 pm, John Firebaugh <[email protected]> wrote: > > > > > > I encounter this situation when I have an external datastructure and > > > > > I need > > > > > to convert it to a filter hash, via inject({}) {...} or similar. With > > > > > the > > > > > current API, I need to create an extra branch in my code to handle > > > > > the empty > > > > > case. > > > > > Are these cases where you wouldn't have access to the dataset? > > > > Because anything possible with a filter hash is going to be possible > > > > with individual filter calls: > > > > > ds = ds.filter(data.inject({}){|d, (k,v)| d[k] = v; d}) > > > > # vs. > > > > ds = data.inject(ds){|d, (k,v)| d.filter(k=>v)} > > > > > > I consider it a code smell when an API does not have a sensible > > > > > behavior for > > > > > obvious boundary conditions in its input. This is of particular > > > > > concern for > > > > > functions like filter that are commonly used in situations where the > > > > > arguments are not a static condition but built dynamically from > > > > > runtime > > > > > input. > > > > > Agreed. Like I said, I'm certainly willing to consider this patch. > > > > The implementation isn't difficult and the performance hit is modest. > > > > Simply because I think there are generally better ways doesn't mean I > > > > think Sequel should violate the POLS. > > > > > BTW, how do you think this should be handled?: > > > > > ds.filter(nil) > > > > > That currently raises an error. I think it should probably use WHERE > > > > NULL (there just isn't a when clause for nil), but I could see some > > > > people thinking filter(nil) should be the same as filter(). > > > > > Jeremy > > -- You received this message because you are subscribed to the Google Groups "sequel-talk" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/sequel-talk?hl=en.
