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

Add prefix option to enum definition #19813

Merged
merged 1 commit into from Jun 12, 2015
Merged

Add prefix option to enum definition #19813

merged 1 commit into from Jun 12, 2015

Conversation

@igas
Copy link
Contributor

@igas igas commented Apr 19, 2015

Fixes #17511 and #17415

/cc @dgsan @chancancode @agis-

@@ -119,8 +132,9 @@ def serialize(value)
attr_reader :name, :mapping
end

def enum(definitions)
def enum(definitions, options = {})

This comment has been minimized.

@kaspth

kaspth Apr 19, 2015
Member

Methods with double hashes are ugly as hell, when you have to surround the first hash with braces. Let's turn prefix into an optional keyword argument and avoid that.

This comment has been minimized.

@igas

igas Apr 19, 2015
Author Contributor

yeah I agree, but first hash is current api =(
Sure, I can change it to keyword arguments.

This comment has been minimized.

@igas

igas Apr 19, 2015
Author Contributor

@kaspth can you please explain how you see api with keyword arguments? cause if we don't want to brake api we can't avoid brackets because any function with more than one argument and first argument is hash need them. Sure we can do def enum(prefix: nil, **definitions) but with that we reserve prefix and user can not use it as enum.

This comment has been minimized.

@kaspth

kaspth Apr 19, 2015
Member

The way I see it prefix will have to go as a possible enum name.
We'll need feedback from others on this though. @rafaelfranca @matthewd @sgrif

This comment has been minimized.

@sgrif

sgrif Apr 19, 2015
Contributor

Either prefix (and presumably suffix) will have to go as possible enum names, or we'll need to use double hashes. I'd prefer to go the double hash route. It's slightly uglier, but the point of this feature is to reduce the number of reserved words. In which case it'd be def enum(definitions, prefix: false, suffix: false)

This comment has been minimized.

@dhh

dhh Apr 28, 2015
Member

Double hash is definitely a no-go. I'm OK with prefix/suffix being reserved keywords for this method under the circumstances.

# the name of the enum.
#
# class Invoice < ActiveRecord::Base
# enum({verification: [:done, :fail]}, prefix: true)

This comment has been minimized.

@kaspth

kaspth Apr 19, 2015
Member

This could easily be written (and be more readable) as:

class Invoice < ActiveRecord::Base
  enum status: [:unverified, :verified]
end

Can we find a better use case for prefix than the above?

This comment has been minimized.

@igas

igas Apr 19, 2015
Author Contributor

In #17511 @dgsan had examples with visibility. It's be better if we define multiple enums where prefix will be needed. Maybe:

enum visible_for: [:admin, :user, :everybody]
enum editable_by: [:admin, :user, :everybody]

or something like this.

This comment has been minimized.

@kaspth

kaspth Apr 19, 2015
Member

Those examples sound like they would be better off with suffixes to me. visible_for_admin? versus admin_visible?

@dhh you're better at making things read nicer. Do you have preference here?

This comment has been minimized.

@dhh

dhh Apr 28, 2015
Member

I like prefix better. It groups them all together. I'd just have gone with visible_to, so it's visible_to_admin?. But I think essentially that you should just be able to choose. enum visible_to: %i( admin user everybody ), suffix: 'visible' would produce admin_visible?.

@@ -138,19 +152,26 @@ def enum(definitions)
_enum_methods_module.module_eval do
pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index
pairs.each do |value, i|
value_prefix = \

This comment has been minimized.

@kaspth

kaspth Apr 19, 2015
Member

Could just be:

method_prefix = "#{prefix == true ? name : prefix}_" if prefix
method_name = "#{method_prefix}#{value}"

This comment has been minimized.

@igas

igas Apr 19, 2015
Author Contributor

I looked at delegate style of prefix implementation. If it matter I'll change it.

This comment has been minimized.

@sgrif

sgrif Apr 19, 2015
Contributor

Why not just do:

if prefix == true
  prefix = name
end

if prefix
  method_name = "#{prefix}_#{value}"
else
  method_name = value
end

I'd rather avoid adding ternaries for something like this

# It is also possible to supply a custom prefix.
#
# class Invoice < ActiveRecord::Base
# enum({verification: [:done, :fail]}, prefix: 'verification_status')

This comment has been minimized.

@kaspth

kaspth Apr 19, 2015
Member

Also we should prefer using symbols as prefixes (but support both symbols and strings).

@s-aida
Copy link
Contributor

@s-aida s-aida commented Apr 19, 2015

I think enum is supposed to get multiple definitions, so options here is kinda global rather than attribute-specific, but passing the prefix for the specific attribute within options is kinda odd.

Of course, you just need to call enum per attribute though.

@igas
Copy link
Contributor Author

@igas igas commented Apr 19, 2015

@shunsukeaida from my experience most of the time people use enum for each attribute and don't know about ability to define multiple. Anyway it's "global" per call, each definition get same prefix. If you don't need prefix or need different you can add more calls. As you mention.

@s-aida
Copy link
Contributor

@s-aida s-aida commented Apr 19, 2015

Fair enough, as I remember until recently Rails even didn't have a test for the multiple enum definitions at once 😏

@rxx
Copy link

@rxx rxx commented Apr 28, 2015

That really has been and closed #13433

@igas
Copy link
Contributor Author

@igas igas commented Apr 28, 2015

@mli-max actually we have updated version =) #17511 (comment)

As @kaspth mention previously we are waiting some feedback from @rafaelfranca @matthewd

I'm personally agree with @sgrif suggestion, we can't avoid current api and best way imo is def enum(definitions, prefix: false, suffix: false)

If everybody agree I'll update PR.

@rxx
Copy link

@rxx rxx commented Apr 28, 2015

@igas Maybe we should add some option to prevent generation methods for a field enum? I need some fields as enum only for inclusion validation for example

@kaspth
Copy link
Member

@kaspth kaspth commented Apr 28, 2015

@dhh I remember you saying you didn't want double hash APIs anymore, so what's your over/under on this?

@kaspth
Copy link
Member

@kaspth kaspth commented Jun 7, 2015

Hey @igas, are you still up for making this happen?

@igas
Copy link
Contributor Author

@igas igas commented Jun 8, 2015

@kaspth yeah, sorry for delay, I'll take a look on this week.

@igas
Copy link
Contributor Author

@igas igas commented Jun 12, 2015

@kaspth hey, can you take a look, please?

@sgrif
Copy link
Contributor

@sgrif sgrif commented Jun 12, 2015

@igas Can you rebase and squash your commits?

@igas
Copy link
Contributor Author

@igas igas commented Jun 12, 2015

@sgrif done

sgrif added a commit that referenced this pull request Jun 12, 2015
Add prefix option to enum definition
@sgrif sgrif merged commit 0158f92 into rails:master Jun 12, 2015
@kaspth
Copy link
Member

@kaspth kaspth commented Jun 12, 2015

@igas nice! ❤️

@@ -75,6 +75,22 @@ module ActiveRecord
#
# Conversation.where("status <> ?", Conversation.statuses[:archived])
#
# You can use <tt>:enum_prefix</tt>/<tt>:enum_suffix</tt> option then you need

This comment has been minimized.

@s-aida

s-aida Jun 12, 2015
Contributor

Am I the only one who thinks it'd rather be when than then?

This comment has been minimized.

@simon2k

simon2k Jun 12, 2015

Right, when would be better, since after this word you have explanation for its usage.

@dhh
Copy link
Member

@dhh dhh commented Jun 15, 2015

Sorry for the late reply here, but I actually think we should just go with _prefix and _postfix. It looks better given the line already starts with enum and it's even clearer that it's a reserved word.

@s-aida
Copy link
Contributor

@s-aida s-aida commented Jun 16, 2015

(...prefix_ and _postfix are better imo)

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

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.