attribute_changed? returns nil (instead of false) for unchanged attributes (4.2.1)

[siebertm]

During an upgrade from 4.1 to 4.2 I noticed that <attribute>_changed? returns nil for unchanged attributes.

On 4.1 and 4.0 it used to return false

IMO (and my colleagues agreed), a "?"-method should always return a boolean.
Do you agree? If so, I'd happily attach a PR with test and fix.

[matthewd]

IMO (and my colleagues agreed), a "?"-method should always return a boolean.
Do you agree?

No.

[eileencodes]

hrm, I tend to agree here - nil is unexpected. @matthewd can you elaborate on why you don't agree?

[matthewd]

http://guides.rubyonrails.org/api_documentation_guidelines.html#booleans

3756a3f #5582 #16521

[siebertm]

But still, it's an API change from 4.1 to 4.2. If that's the core team's opinion, then, OK. I'll just work around that bug (IMO this still is a bug).

Thanks!

[matthewd]

@siebertm it's officially not an API change because the method was never documented as returning false. Any code expecting it to (or holding the same expectation for any other similar method) is relying on undocumented behaviour... just the same as calling a private/undocumented method.

[siebertm]

@matthewd Don't understand me "insisting" on being right, but when I look at the docs for ActiveModel::Dirty, at least the examples clearly document false: http://api.rubyonrails.org/classes/ActiveModel/Dirty.html

Since these dynamic methods are not documented elsewhere, I (the user) get the impression that it returns either true or false, since also the other documented methods state this, e.g. changed? which

• at least for me - comes as close as it can to the dynamic ones.

[matthewd]

Good point.

@fxn @rafaelfranca?

[rafaelfranca]

Yeah, that is the tricky part of our policy for boolean, the code examples will be sometimes not accurate. I'd say that what we want to document there is false not false, but it is hard to say that in a code example since we need to document some return.

In this case I'd say that our policy of using boolean semantics instead of exactly values have precedence.

[chancancode]

@siebertm Let me fill you in with some history! 😄

Like Matthew and the article suggested, this (whether predicate methods ending in a ? should guarantee a true or false return value, in general) has been discussed many times on Github and mailing lists. In the grand scheme of things, it's Not That Important™ relative to the time the cumulative time spent on discussing this, so at some point an Executive Decision™ was made in order to stay productive.

The decision that was made was that "No, they should only guarantee a truthy/falsy return value, i.e. anything that is not {false/nil} for YES, and either {false/nil`} for NO".

The main arguments are:

• This is how Ruby views truthiness in general. Checks like something? == true is explicitly frowned upon in idiomatic Ruby. (See also https://www.ruby-forum.com/topic/4412411#1103364)
• If we assume that our users would write idiomatic Ruby (which we should), then there is really no benefit in guaranteeing a true/false return value.
• By guaranteeing true/false return values, we would have to "litter" our code with !! everywhere, slows everything down by a tiny bit, makes refactoring more difficult, all for something that no one would actually benefit from if they are using it "correctly".

So that was why the policy was made.

Now, there has been some difficulties in terms of executing said policy, mostly in "how should we document these methods". No matter what you do, it is difficult to make this history and policy obvious to the casual reader that didn't start reading the manual from page zero (which is totally understandable).

What we try to do is:

• Avoid saying "returns true/false"
• Distinguish true/false from "truphy"/"falsy" by referring to the later as "true/false" (without the fixed-width font)
• @returns Object
• ...
• See http://guides.rubyonrails.org/api_documentation_guidelines.html#booleans

Occasionally, we let these things slip into the documentation and ended up in a released version. In those cases we view that as a mistake on our part (that we accidentally promised something we shouldn't in the docs) so we change the code to return true/false and lock it down with a test.

The code example case is tricky like Rafael mentioned, and I don't think we have a good way to handle that ambiguity, so it's kind of a grey area. I suppose we need to make another Executive Decision on this and how to make it more obvious going forward.

For that, @fxn is probably the best person who is in the position of making that call. So I'll reopen the issue and let him decide one way or other or chime in with some more details.

Once this settles, if you have other ideas to improve this in the right direction (i.e. make the docs clearer, not changing the code across the project), feel free to suggest them. I'm sure the documentation/guides could use some improvements on this topic. It'll help others avoid going down the pit you stumbled into in the future 😄 👍

[fxn]

@chancancode hat tip, awesome summary.

Regarding this particular PR, in the past sometimes we've acknowledged that the code example return values could make people think singletons were guaranteed, I remember 126dc47 for example. (Please ignore the comment about YARD.)

I think the only way out of this situation is to document Object as return type. If the signature of the method clearly says Object, then code examples can say whatever. Though there's a little inner voice in me that whispers "truthy"/"falsey" could be even less ambiguous in examples, no matter how much I find these terms unnecessary because Ruby defines what is true and false in the language. It's a trade-off, if Object is unambiguosly documented as return type, that may be enough.

Since Ruby does not have a Boolean class, another option would be to use Boolean to mean "Object, to be interpreted in a boolean context", but I believe some users would assume Boolean means true/false. Not sure about that one, Object seems to settle any doubts.

The problem to implement a policy like that today is that we don't have a formal/structured/predictable way to document signatures. Our last plan with @zzak was to explore something really simple based on :call-seq:, similar to how the Ruby API uses it. We should move on with this, maybe I could try to work on this for 5.

[siebertm]

Thanks for the explanation @chancancode and @fxn!

Also, to make you understand why I openend this ticket:

We have a model with a boolean column and a NOT NULL constraint. In the model we call:

def do_something
  self.boolean_column = foo_changed? || bar_changed?
end

So, when the _changed? predicates return nil (which is correct ruby-ism I know that), the result is self.boolean_column being set to nil which of course raises an error.

The tests for that used to work till we upgraded to 4.2.

What we came up with was either using present? (which isn't "safe" because per your policy it returns something truthy or falsey, including maybe nil) or add an || false:

def do_something
  self.boolean_column = foo_changed? || bar_changed? || false
end

[fxn]

@siebertm that's the point: when the API only guarantees boolean semantics, client code needs to get singletons if it totally needs them (rare). In code like yours, !! is the usual trick:

self.boolean_column = !!(foo_changed? || bar_changed?)

Looks ugly eh? As Godfrey explained above that's precisely one of the reasons we don't generally commit to singletons, because we don't want those !!s in Rails code, and because most of the time client code won't need them either so we are not passing the ugliness up.

That's a client code code responsibility, because it is client code who needs to transfer types across different technologies. Like serializing a boolean value to be interpreted by JavaScript: Since JavaScript has different boolean semantics, you are responsible for ensuring the serialization is going to be interpreted correctly. (That was the start of the thread @matthewd linked above.)

Returning to this issue, in the case http://api.rubyonrails.org/classes/ActiveModel/Dirty.html, there's little information but code examples. And changed? documents singletons. I believe in this case it would be reasonable to patch the attribute predicates to return singletons, as a user I could have interpreted that's the API.

As per a general policy, I think if the the main docs only document boolean semantics, examples of course do not override that. If we adopted that policy, then we wouldn't be able to document Object in many predicates, because they generally have examples.

[jmondo]

@siebertm side note. if you slap a null: false on your boolean columns, this type of thing won't catch you by surprise. instead you'll get (for PG): ActiveRecord::StatementInvalid: PG::NotNullViolation: ERROR: null value in column "boolean_column" violates not-null constraint. while you're at it a default: false is also probably a good idea.

[jamesprior]

FWIW I also ran into this and found it a surprise. It was not a big fix, I had state_id_changed? == false which I prefer for readability. I changed it to !state_id_changed?which I find less readable.

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

[rails-bot]

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.