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.
