Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idea: Predicate method for Boolean values #15

Closed
mscoutermarsh opened this issue Mar 24, 2016 · 10 comments
Closed

Idea: Predicate method for Boolean values #15

mscoutermarsh opened this issue Mar 24, 2016 · 10 comments

Comments

@mscoutermarsh
Copy link
Contributor

Hello, love this gem :)

What do you think of auto defining predicate methods for boolean values? (to match what Rails does for booleans).

some_setting Boolean, default: true

object.some_setting? # => true

If you like this idea, I'd be happy to implement.

@ramontayag
Copy link
Contributor

Oh hey that's cool! Yeah sure please give it a shot. There's a lot of metaprogramming there (kinda needed to) so I hope you find your way around.

@stebo
Copy link
Contributor

stebo commented Aug 1, 2016

What do you think about simply solving this like here: https://github.com/stebo/storext/commit/9335e5d23c395f509c13f67c4afd7166ee984d3a

@stebo
Copy link
Contributor

stebo commented Aug 1, 2016

Edit: I just came up that it maybe would be better to add the predicate method for all type of values, not only Booleans (which Rails is also doing for Strings, Integers...), so my commit could be simplified even more by just adding

alias_method "#{attr}?", attr

in line 22...

So you could e.g. also check if a book has a title with

book.title? # => true if title is set

@ramontayag
Copy link
Contributor

@stebo yes look like your commit will work for booleans.

About your second comment: won't that just return the value? For example, if #age returns 25, then wouldn't #age? also return 25? I think we have to test for presence. But then, should an empty string ("") return true or false? Perhaps the least surprising implementation would be anything that is nil or false will return false. Anything not nil or true will return true. Do I make sense?

@stebo
Copy link
Contributor

stebo commented Aug 2, 2016

@ramontayag ahhh of course you are right, did not really made a test for non-boolean values.
So here is a new proposal. We add this method

def storext_define_predicater(column, attr)
  define_method "#{attr}?" do
    if send(column) && send(column).has_key?(attr.to_s)
      store_val = !!read_store_attribute(column, attr)
      # storext_cast_proxy.send("#{attr}=", store_val)
    else
      false
    end
    # storext_cast_proxy.send("#{attr}")
  end
end

and call it in def storext_define_accessor after storext_define_reader.

  1. the two commented cast_proxy lines could be deleted and are not needed here, or?
  2. the "else false" is necessary because otherwise attributes which are defined without a default value and are not yet existing in the json/hstore Hash, will result in
object.attribute => nil
object.attribute? => nil # and we want false here 

I think we could than shorten the method to

def storext_define_predicater(column, attr)
  define_method "#{attr}?" do
    false unless send(column) && send(column).has_key?(attr.to_s)
    !!read_store_attribute(column, attr)
  end
end

What do you think?

@ramontayag
Copy link
Contributor

Yes I think that will work! return false though, so it doesn't continue.

@stebo
Copy link
Contributor

stebo commented Aug 4, 2016

Good hint, I included that. Here is the pull request: #26

@ramontayag
Copy link
Contributor

Thanks for the contribution!

@ramontayag
Copy link
Contributor

v2.2.0 has been released with this change

@mscoutermarsh
Copy link
Contributor Author

😄 😄 awesome @stebo @ramontayag !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants