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

ActiveRecord::Enum, model.status_was return a String value, not integer #13267

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@huacnlee
Contributor

huacnlee commented Dec 11, 2013

class Conversation < ActiveRecord::Base
  enum status: { active: 0, archived: 1 }
end

Before:

conversation.status = :active
conversation.status = :archived
conversation.status_was # => 0

After:

conversation.status_was # => "active"
@senny

This comment has been minimized.

Member

senny commented Dec 11, 2013

We can't simply change a single method from ActiveModel::Dirty. You expect the whole dirty tracking API to work the same. See ActiveModel::Dirty

@senny

This comment has been minimized.

Member

senny commented Dec 12, 2013

@huacnlee are you updating the PR so that all AM::Dirty methods will return strings?

@huacnlee

This comment has been minimized.

Contributor

huacnlee commented Dec 12, 2013

@senny This changes only effect with enum in enable.

I just want _was method return a String if it an enum key. not a number, because I was set a String value.

@huacnlee

This comment has been minimized.

Contributor

huacnlee commented Dec 12, 2013

Sorry, I don't know what means of your first reply.

@senny

This comment has been minimized.

Member

senny commented Dec 12, 2013

@huacnlee as mentioned previously, we can't simply overwrite a single method from AM::Dirty. It needs to behave consistently. That means methods like #changes, #enum_change must also return Strings and not Integers. This problem is solved by feeding AM::Dirty the String values and not the Integers.

@huacnlee

This comment has been minimized.

Contributor

huacnlee commented Dec 12, 2013

@senny Ok,I probably know, and I wil try to change it

@wyaeld

This comment has been minimized.

Contributor

wyaeld commented Dec 19, 2013

Why would you want it returning a String, when you actually set the value as a Symbol?

conversation.status = :active
conversation.status = :archived
conversation.status_was => "active" # wtf
conversation.status_was => :active  # this is symmetrical
@senny

This comment has been minimized.

Member

senny commented Dec 19, 2013

@wyaeld assigned symbols are casted to strings. Enum internally works with strings so that's what you can expect from the API to be returned.

@wyaeld

This comment has been minimized.

Contributor

wyaeld commented Dec 19, 2013

I get that its using strings internally, and I assume you can feed it actual strings instead of symbols and have it work with that, my issue is that such asymmetrical behaviour in general violates the Principle of Least Surprise, and adds a degree of confusion to people either new to the framework, or new to the feature.

@senny

This comment has been minimized.

Member

senny commented Dec 20, 2013

@wyaeld I disagree. If only String values can be assigned, Symbols are casted into a String. Then it's expected that enum_was will always return a String. _was returns the last stored value. We have that exact behavior already for other types. Consider:

irb(main):004:0> n.event_id_was
=> 1
irb(main):005:0> n.event_id="7"
=> "7"
irb(main):006:0> n.save!
=> true
irb(main):007:0> n.event_id = "1"
=> "1"
irb(main):008:0> n.event_id_was
=> 7
@wyaeld

This comment has been minimized.

Contributor

wyaeld commented Dec 20, 2013

I don't think I fully understand, but I'm happy enough if things are being done to maintain consistency with existing behaviour

@chancancode

This comment has been minimized.

Member

chancancode commented Dec 20, 2013

@wyaeld the summary is conversation.status will return a string. Unless that changes, conversation.status_was should also return the same type (a string)

@barelyknown

This comment has been minimized.

Contributor

barelyknown commented Dec 25, 2013

Is anyone working on this? If not, I can write it today.

@barelyknown

This comment has been minimized.

Contributor

barelyknown commented Dec 26, 2013

This PR has an upstream fix for this. #13489

@senny

This comment has been minimized.

Member

senny commented Dec 27, 2013

@barelyknown thanks. I'm giving this PR a close.

@senny senny closed this Dec 27, 2013

@senny senny added the enum label Sep 4, 2014

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