riak-ripple / ripple

A rich Ruby modeling layer for Riak, Basho's distributed database
Other
618 stars 152 forks source link

Allow index blocks/methods to return nil, excluding index entry #288

Closed seancribbs closed 10 years ago

seancribbs commented 12 years ago

This allows for patterns where the index should not be available on some documents that fail to meet certain criteria. I'd appreciate arguments for/against this change. Some caveats to start the discussion:

/cc @stevenbristol

ntalbott commented 12 years ago

How will this affect indexing boolean flags? Especially if the flag is sometimes nil, but that should actually be indexed as a false value?

seancribbs commented 12 years ago

@ntalbott Good question. In what case where you would want a boolean property to be nil?

ntalbott commented 12 years ago

When I assign a flag, I don't tend to worry whether or not I'm assigning it with nil vs. false - I just assign it with a truthy/falsy value and leave it. Does Ripple cast it to an actual true/false value?

seancribbs commented 12 years ago

This is the path it goes through: https://github.com/seancribbs/ripple/blob/master/lib/ripple/core_ext/casting.rb#L65

The question is more for indexes that are custom-defined I guess, or where the property is not boolean.

ntalbott commented 12 years ago

So that path will leave a nil value alone, which makes the case of a flag field an issue, as I often don't explicitly set a flag but instead just count on it starting out nil (and therefore false).

I guess checking nil just feels hackish and could cause surprises; would something like the if:/unless: checks on callbacks be a better fit here?

index :name_age, String, if: proc{|object| (object.name && object.age)} do
  "#{self.name}-#{self.age}"
end
myronmarston commented 12 years ago

I don't think I like nil having a special implicit meaning, either. nil is the catch all undefined value that means very different things in different contexts. As @ntalbott pointed out, it means false in a boolean context.

I like Nathaniel's suggested if/unless` idea better than using nil. Alternately, you might consider having the block yield an object that provides an API for this:

index :name_age, String do |indexer|
  indexer.skip! if name.nil? || age.nil?
  [name, age].join("-")
end

One advantage of this approach: it's potentially more flexible for other future special-casing we want to do. The indexer object can expand to support as many "index controlling" methods as we want.