simple_enum icon indicating copy to clipboard operation
simple_enum copied to clipboard

Ability to create selectors passing symbols

Open lucke84 opened this issue 10 years ago • 24 comments

Let's say we have a simple Mongoid model:

class Article 
  include SimpleEnum::Mongoid

  VALID_TYPES = { blog: 0, vlog: 1, xlog: 2 }

  # ...
  as_enum :type, VALID_TYPES, source: :type
end

As far as I can see (correct me if I'm wrong!), it's not possible to query using a selector that takes the symbol corresponding to a value of an enum, like this:

Article.where(type: :blog).count

while instead is possible to do this:

Article.where(type: 0).count
Article.where(type: Article::VALID_TYPES[:blog]).count # or this, don't like it though

I'm aware of the fact that it's possible to use chained selectors, like this:

Article.blogs.count

but in our codebase we often build selectors as hashes that are passed to instances of different classes.

Besides this maybe not being the best approach of all times, IMHO it'd be very beneficial to be able to consume the gem in a consistent way, always passing the symbol (key of the hash), not the value.

Do you think it's a complicated matter? Can you give me pointers to where to look at, so maybe I can submit a PR?

lucke84 avatar Jan 20 '15 13:01 lucke84

That can actually become nasty !

User.find_or_create_by(name: "Test user", gender: :male)

... I was wondering why I had so many users in my test database :P

(as_enum: :gender)

Startouf avatar Feb 16 '15 18:02 Startouf

Personally I really think we should not override things like e.g. where and mess with how selection works.

lwe avatar Feb 17 '15 14:02 lwe

You're right. But maybe we could define an extra convenience method that preprocesses the inputs so as to replace "meaningful enums" by their "meaningless database representation"

Something like that

def preprocess_enums_in_hash(class, hash={})
  preprocessed_hash = {}
  hash.each do |key,val|
    # The key corresponds to an enum value ? if yes convert it to {key_cd, integer_value_of(value)}
    # EX : {gender: :male} should be converted to {gender_cd: 0} because we know :gender is an enum declared in the class class
    if is_enum_of(class, key)? 
      enum_symbol = "#{key.to_s}_cd" # Also handle the case where enums are prefixed
      enum_value = integer_value_of_enum(class, enum_symbol, val)
      preprocessed_hash << {:enum_symbol => enum_value}
    else
      # Don't modify the current (key, val) of the hash
      preprocessed_hash << {key => val}
    end
  end
  preprocessed_hash
end

Then

Person.where(preprocess_enums_in_hash(Person, {age: 21, gender: :male}

Would become

Person.where(age: 21, gender_cd: 0}

Startouf avatar Feb 17 '15 14:02 Startouf

All right if somebody can start a Pull Request I'm happy to merge, one thing I'd prefer it to be optional, so that an additional file needs to be required or something and that it registers itself as extension.

See https://github.com/lwe/simple_enum/blob/e65073193ed4a50bb1bac365b1620eb5a8123650/lib/simple_enum/attribute.rb#L85-96

Update: from an "API" perspective I'd rather have it be something like Person.expand_enums_in_hash, rather than an external utility, or not? Also a good / clear method name needs to be found :smile:

lwe avatar Feb 18 '15 08:02 lwe

I vote for the Person.expand_enums_in_hash approach. The other option (preprocess_enums_in_hash(Person, {age: 21, gender: :male})) would feel unnatural to use (and hard to read).

As per the name, something like .enums_to_selector?

lucke84 avatar Feb 19 '15 13:02 lucke84

I vote for expand_enums_in_hash (unless better choice appears)

.enums_to_selector makes it feel like we're going to use the hash for a DB query, which isn't necessarily the case ? (Haven't used SimpleEnum outside the scope of mongoid/active_record, but then it's possible)

In my mind, it was external because it reminded me of enum_option_pairs(class_name, enum), but it does make more sense to have Person.expand_enums_in_hash

Startouf avatar Feb 25 '15 01:02 Startouf

@Startouf as per the nature of the gem, I was imagining it used for DB queries only, otherwise a simple to_hash would be enough.

Also, in my mind, the method should also take care of dealing with lists of values and translate them into the right DB selector, e.g.:

Person.enums_to_selector(gender: :male)             # --> { gender: 0 }
Person.enums_to_selector(gender: [:male, :female])  # --> { :gender.in => [0, 1] }

Hence the name I came up with.

Does this make sense to everybody?

lucke84 avatar Feb 25 '15 17:02 lucke84

We should go with enums_to_selector, because as @lucke84 pointed out this method is only ever used in combination within a where clause.

lwe avatar Feb 25 '15 20:02 lwe

I believe ActiveRecord/Mongoid are already doing the transformations from {gender: [0, 1]} to {:gender.in => [0,1]}

Startouf avatar Feb 25 '15 20:02 Startouf

Yes, correct, I think at the moment we should focus on the following:

{ gender: :male } # => { gender: 0 }
{ gender: [:male, :female] } # => { gender: [0, 1] }
{ foo: "bar", gender: :female } # => { foo: "bar", gender: 1 }

lwe avatar Feb 25 '15 21:02 lwe

Do we need the inverse operation ?

.enums_to_int / .int_to_enums
.expand_enums
.meaningless (enum to int) /.meaningful (int to enum)
.as_int / .as_enum (name collision ?)

Startouf avatar Feb 25 '15 21:02 Startouf

No, I don't think we need the inversions.

lwe avatar Feb 25 '15 21:02 lwe

An intriguing idea was given in #101, to basically add a method like

Klass.gender_scope(:female) # => where(gender_cd: 1)

this could be extend to allow both single arguments or hashes, with the expansion / support as described above?

Klass.gender_scope(gender: :male, foo: "bar") # => where(gender_cd: 0, foo: "bar")

lwe avatar Apr 29 '15 19:04 lwe

One thing that often happens to me is to recycle a selector (not a scope) with different classes that extend/include a certain field.

The main purpose is to be able to pass a selector to another method without necessarily knowing which class is going to use it (something you can do with whatever other key/value pair).

Example:

class A
  STATUSES = [:active, :paused, :inactive]
  as_enum :status, STATUSES, source: :status
end

class B
  include A
  # more fields
end

class C
  include A
  # more fields
end

Somewhere else:

selector = { status: A::STATUSES[:active], another_field: 'abc' } # ideally it'd be `{ status: :active, another_field: 'abc' }`
x = B.where(selector.merge({ yet_another_field: true })).count
y = C.where(selector.merge({ one_more_field: 25 })).first

I don't think the suggested method would make easier achieving such goal, unless we provide also the ability to get the selector only.

lucke84 avatar Apr 29 '15 23:04 lucke84

@lucke84 why isn't it possible with the ..._scope to reuse selectors? Like e.g.

selector = { status: :active, field: 'abc' }
x = B.xxx_scope(selector.merge(yet_another_field: true)).count
y = C.xxx_scope(selector.merge(one_more_field: 25)).first

lwe avatar Apr 30 '15 07:04 lwe

That's right, it should work. Cool!

Can we also think about adding support for a generic enum_scope (or where_with_enum, or else) that doesn't necessarily know which field is gonna be involved:

e.g.:

Klass.where_with_enum({ status: :active, another_enum: :something }).first

instead of

Klass.status_scope({ status: :active, another_enum: :something }).first

lucke84 avatar Apr 30 '15 16:04 lucke84

Hey, so I introduced status_scope on my project simply because I joined this team a few weeks ago, and everywhere I saw

Project.where(
  status_cd: [
    Project.statuses(:pending),
    Project.statuses(:active)
  ])

So I added the scope, and now I can just say

Project.status_scope([:pending, :active])

If I want to chain these

Project.status_scope(:active).language_scope(:ruby)

I can see why it might be cool to write in a single call

Project.where_with_enum(status: :active, language: :ruby)

(In my head I'd call it enum_scope, but I think I'm too old skool)

matthewrudy avatar May 03 '15 21:05 matthewrudy

I should also say, I'm well into making sure that we raise an exception for any invalid values.

ie.

Project.status_scope(:doesnt_exist)

and

Project.where_with_enum(status: :doesn't_exist)

should both raise an InvalidEnum exception.

matthewrudy avatar May 03 '15 21:05 matthewrudy

+1 @matthewrudy that is exactly my point :)

lucke84 avatar May 03 '15 22:05 lucke84

@matthewrudy, wow, nice work 👍 and I assume you basically delegate xyz_scope to where_with_enum(xyz: ...)?

Any chance you can already create WIP pull request so we can take a look at the code?

lwe avatar May 04 '15 06:05 lwe

I might be too late but i would prefer

  1. remove default scopes, ex: Project.pendings
  2. add a single enum scope Project.with_enum(status: :pending).where(...)

elfassy avatar Sep 29 '15 15:09 elfassy

:+1: thanks for the input and no you are not too late, development of v3 has been kind of stale recently, sorry for that.

Update and my tendency is also towards with_enum and not single scopes like e.g. pendings

lwe avatar Sep 29 '15 15:09 lwe

I am for not having single scopes too. Has any work been done in this direction?

aledalgrande avatar Apr 11 '17 01:04 aledalgrande

@aledalgrande, no AFAIK no work has been done into this direction yet.

lwe avatar Apr 11 '17 11:04 lwe