Add prefix option to enum definition#19813
Add prefix option to enum definition#19813sgrif merged 1 commit intorails:masterfrom igas:enum-prefix
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah I agree, but first hash is current api =(
Sure, I can change it to keyword arguments.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Double hash is definitely a no-go. I'm OK with prefix/suffix being reserved keywords for this method under the circumstances.
|
I think Of course, you just need to call |
|
@shunsukeaida from my experience most of the time people use |
|
Fair enough, as I remember until recently Rails even didn't have a test for the multiple enum definitions at once 😏 |
|
That really has been and closed #13433 |
|
@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 If everybody agree I'll update PR. |
|
@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 |
|
@dhh I remember you saying you didn't want double hash APIs anymore, so what's your over/under on this? |
|
Hey @igas, are you still up for making this happen? |
|
@kaspth yeah, sorry for delay, I'll take a look on this week. |
|
@kaspth hey, can you take a look, please? |
|
@igas Can you rebase and squash your commits? |
|
@sgrif done |
Add prefix option to enum definition
|
@igas nice! ❤️ |
There was a problem hiding this comment.
Am I the only one who thinks it'd rather be when than then?
There was a problem hiding this comment.
Right, when would be better, since after this word you have explanation for its usage.
|
Sorry for the late reply here, but I actually think we should just go with |
|
(... |
Unlike other features built on Attribute API, reserved options for `enum` has leading `_`. * `_prefix`/`_suffix`: rails#19813, rails#20999 * `_scopes`: rails#34605 * `_default`: rails#39820 That is due to `enum` takes one hash argument only, which contains both enum definitions and reserved options. I propose new syntax for `enum` to avoid leading `_` from reserved options, by allowing `enum(attr_name, ..., **options)` more Attribute API like syntax. Before: ```ruby class Book < ActiveRecord::Base enum status: [ :proposed, :written ], _prefix: true, _scopes: false enum cover: [ :hard, :soft ], _suffix: true, _default: :hard end ``` After: ```ruby class Book < ActiveRecord::Base enum :status, [ :proposed, :written ], prefix: true, scopes: false enum :cover, [ :hard, :soft ], suffix: true, default: :hard end ```
8 participants
Fixes #17511 and #17415
/cc @dgsan @chancancode @agis-