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

Ability to create selectors passing symbols #95

Open
lucke84 opened this issue Jan 20, 2015 · 24 comments
Open

Ability to create selectors passing symbols #95

lucke84 opened this issue Jan 20, 2015 · 24 comments

Comments

@lucke84
Copy link

lucke84 commented Jan 20, 2015

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?

@Startouf
Copy link

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)

@lwe
Copy link
Owner

lwe commented Feb 17, 2015

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

@Startouf
Copy link

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}

@lwe
Copy link
Owner

lwe commented Feb 18, 2015

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 😄

@lucke84
Copy link
Author

lucke84 commented Feb 19, 2015

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?

@Startouf
Copy link

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

@lucke84
Copy link
Author

lucke84 commented Feb 25, 2015

@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?

@lwe
Copy link
Owner

lwe commented Feb 25, 2015

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

@Startouf
Copy link

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

@lwe
Copy link
Owner

lwe commented Feb 25, 2015

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 }

@Startouf
Copy link

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 ?)

@lwe
Copy link
Owner

lwe commented Feb 25, 2015

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

@lucke84 lucke84 mentioned this issue Apr 29, 2015
3 tasks
@lwe
Copy link
Owner

lwe commented Apr 29, 2015

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")

@lucke84
Copy link
Author

lucke84 commented Apr 29, 2015

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.

@lwe
Copy link
Owner

lwe commented Apr 30, 2015

@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

@lucke84
Copy link
Author

lucke84 commented Apr 30, 2015

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

@matthewrudy
Copy link

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
Copy link

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.

@lucke84
Copy link
Author

lucke84 commented May 3, 2015

+1 @matthewrudy that is exactly my point :)

@lwe
Copy link
Owner

lwe commented May 4, 2015

@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?

@elfassy
Copy link

elfassy commented Sep 29, 2015

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(...)

@lwe
Copy link
Owner

lwe commented Sep 29, 2015

👍 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

@aledalgrande
Copy link
Contributor

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

@lwe
Copy link
Owner

lwe commented Apr 11, 2017

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

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

No branches or pull requests

6 participants