request.xhr? now returns boolean as expected, rather than nil or 0

[clifton]

This was referenced in #1817 and I did run into a small problem migrating a very large application from Rails 2.3 -> Rails 3.x.

Does it not make an API confusing to have #some_method? return a non-boolean value, even if it works in most places as expected?

Here's a very contrived example (obviously you would never need to do this as written).

# ruby
{:xhr => request.xhr?}.to_json

// javascript that works in 2.3
if (result.xhr) console.log("Never executes in rails >= 3.1");

[carlosantoniodasilva]

/cc @fxn

[fxn]

I am -1 on this one. Predicates return a value that is true or false in the Ruby definition of the words and client code should not expect any particular value from them (unless documented).

You write

foo unless request.xhr? # GOOD

rather than

 foo unless request.xhr? == true # DON'T DO THIS

The singletons allow you to return booleans when you need them.

[clifton]

Well, to be fair, github.com/styleguide from @kneath

The names of predicate methods (methods that return a boolean value) should end in a question mark. (i.e. Array#empty?).

No rubyist would ever write unless request.xhr? == true and the part of our app that broke had some parameterization passed similar to :ajax_request => request.xhr?, which TBH isn't great style, but would break down when testing for true or false in a language such as javascript where 0 evaluates to false.

And regardless of Ruby style opinions (obviously I'm in the camp that thinks it's bad style to not return true or false from a #method?) it's an API change coming from 2.3. In 2.3 it returned true or false. API changes that happen without good reason are only going to frustrate your users.

[fxn]

The code

 :ajax_request => request.xhr?

is broken. It is not using the predicate as such, it is relying on the undocumented return value of the predicate. Rails didn't promise any value and thus there's no contract changed: The contract is that the value is true or false (regular font) depending on whether the request is Ajax.

If you need a boolean as value for :ajax_request you need to get one out of the predicate, for example

 :ajax_request => !!request.xhr?

or something more explicit.

[clifton]

Well, to fix the code during our rails migration (which still isn't complete after 3 months -- 75k LOC app), that's exactly what I had to do.

Rails 2.3 version

:key => request.xhr?

Rails 3.1 version:

:key => !!request.xhr?

My two arguments, , were:

• This needlessly broke API compatibility between 2.3 and 3.1
• Using !!some_method? is bad style, I understand using !!some_method but the ? should indicate a boolean return value.

The second argument might be nitpicky (and I suppose just my opinion), but the first isn't.

[fxn]

I understand that having something subtle to fix is annoying.

You've gone over a major upgrade though, and again, the interface of the predicate has not changed at all anyway!

You have to understand that ? in xhr? does indicate a boolean value. You believe it should be true or false (fixed-with font), but in Ruby any value is a boolean value because the language defines what is true and false.

And that is the way one writes predicates in Ruby.

Also operators rely on this language feature

if 1 && 2 && 3
  # do something
end

enters the loop because 1 && 2 && 3 evaluates to the true value 3, there's no requirement for && to return true or false (and it doesn't). Same for =~ which is what xhr? is based on today.

That's the way Ruby works! These semantics about predicates are very common in programming languages indeed.

OK the thread is clear I believe. I am convinced of the criteria to apply here and that the code had no business doing a double negation. So I am going to close the pull request and at the same time understand your opinion may be different.

[masterkain]

Sorry but I concur with @clifton, request.xhr? should be casted to return true or false. my 2 cents.

[fxn]

@masterkain predicates in Ruby are not required, nor expected, to return any particular value. Client code cannot depend on the value either, unless documented (like Integer#nonzero?). The code in that application was subtly using the returned value. That's broken, nobody is telling anything about the return value.

The singletons true and false are there so that you can use them in predicates if nothing else makes sense.

For example, you'd write

def has_name?
  first_name && last_name
end

you could also wirte

def has_name?
  first_name && last_name ? true : false
end

but that's redundant and nobody requires/expects the predicate to be written that way.

[masterkain]

@fxn whilst I agree with your point my thinking goes before the entire 'what should we return from a method with?'.

I mean, POLS (http://en.wikipedia.org/wiki/Principle_of_least_astonishment). We are talking about a Rails method that it's used for its true/false values and nothing else.

[clifton]

# returns true or false
def has_name
  (first_name && last_name).present?
end

Just an example of using #present? which always returns true or false. I understand your argument of whether it should be guaranteed to return a boolean or just work in a conditional statement, but again it did break API compatibility and it looks stylistically ugly to some of us to return a non-boolean value.

[fxn]

Let's be crystal clear: no API has been changed at all

http://api.rubyonrails.org/v2.3.8/classes/ActionController/Request.html#M000518

Please stop making that claim and realize your app, as annoying as it can be, had a time bomb! The reaction should be: "Ouch, look we were relying on a value that was not guaranteed. Also, we were sending a non-portable value through the wire rather than ensuring it was portable. Good this upgrade made this apparent, let's fix it".

[tarcieri]

"it is relying on the undocumented return value of the predicate"

I'd almost say it's a failure of Ruby's predicate methods not to coerce to a boolean and instead presenting "truthy"/"falsey" values. These can be quite confusing from irb.

+1 on coercing to true/false unless there's a legitimate performance concern which dictates otherwise. Returning nil and 0 is especially confusing considering that 0 is "truthy" in Ruby and false in most other languages.

[fxn]

Sorry, it seems I edited a comment (thought I was replying).

@clifton said the docs say otherwise because it contain the word "true". Response is:

Of course not.

Being true (regular font), means being true. Being false means being false (regular font). All values in Ruby are true except false and nil.

[wagenet]

It seems like this PR is being turned down on principle vs practicality. I can't see how there is any problem with coercing the value to true or false even if that shouldn't strictly be required by Ruby conventions.

[tarcieri]

@fxn: They're not true. They're truthy. Is truthiness good enough? Maybe for Stephen Colbert, but not for me. Please don't let truthniness replace what's actually true.

[fxn]

Gents, you may have personal preferences regarding how you write your predicates. That's perfect.

But the fact is that Ruby the programming language has clear rules about this. As most programming languages out there.

Predicates typically return one of the Boolean values true or false, but this is not required, as any value other than false or nil works like true when a Boolean value is required. (The Numeric method nonzero?, for example, returns nil if the number it is invoked on is zero, and just returns the number otherwise.)

That's from Flanagan & Matz.

Thus, while you may have personal preferences and reflect them on your code, when your code is client code you cannot expect anything particular from a predicate unless documented.

That's the way the language works.

[vertis]

@fxn you're wrong about this

!(@env['HTTP_X_REQUESTED_WITH'] !~ /XMLHttpRequest/i)

returns a true/false. Which is a well documented and accepted convention for methods that end in a '?'. The new method as found in 3.1 has broken a contract, indeed the documentation you linked to makes it very clear. "Returns true if the request‘s "X-Requested-With" header contains "XMLHttpRequest"...".

Changing that behaviour and then pushing the blame onto the user is not right.

[tarcieri]

Returning 0 as an indicator of truthiness is wrong. That's not personal preference. That's common sense.

[clifton]

http://api.rubyonrails.org/v2.3.8/classes/ActionController/Request.html#M000518 :

xml_http_request?()

Returns true if the request‘s "X-Requested-With" header contains "XMLHttpRequest". (The Prototype Javascript library sends this header with every Ajax request.)

This method is also aliased as xhr?

I suppose my last comment was moderated. Really?

[kneath]

Regardless of ruby style, mandates, API documentation semantics... would Rails be easier to work with if this method returned true/false?

Personally I would say yes. I would love to see this merged. Seems like it would keep backwards compatibility and improve the future.

[clifton]

thank you @kneath 👏👏👏👏👏

[tarcieri]

+1 there @kneath

[fxn]

@vertis wrong, the entire Rails documentation uses true and false to indicate that. That's the normal way to describe a predicate "says whether...", "returns true if...".

When we want to say the singleton true or false (which is totally unusual because it is almost never needed) we use fixed-width font.

Why that convention? Because it is the convention used in Ruby predicates. You use them in conditional statements and such, never use their value unless documented.

[bionicpill]

I think the crux of this problem is the example is trying to coerce Ruby's idea of truthiness to Javascript's idea of truthiness and they are far from aligned. The issue is not in the Ruby code which I believe is doing the correct thing. Duck typing is awesome because we don't have to force things to be true or false. If this breaks your JS, well then you need to write a smarter converter. Trying to force Ruby to be compatible with other languages sets a very bad precedent.

[aprescott]

@tarcieri +1 on distinguishing truthiness from true.

If #xhr?'s return value shouldn't be relied upon except as either something truthy or falsy in a condition, then why was xhr? changed to 0 and nil in 0aa66f0?

[fxn]

@tarcieri having 0 to indicate something is true in Ruby is wrong? Can you please enumerate which are the false values in Ruby?

Or is Rails written in C?

[fxn]

@aprescott because that change does not alter the contract, and removes a double negation (one in an exclamation mark, and another one in the operator).

[tarcieri]

But seriously, I've been doing this Ruby thing for awhile, and returning 0 from a predicate method to mean true is extremely confusing.

Things that you could return that are better than 0:

• true
• 1
• 42
• :yes
• /XMLHttpRequest/
• "This is indeed an XMLHttpRequest"

Any of those would be acceptable. Plzfix.

[mboeh]

"Ruby doesn't require us to do X so we won't do X" isn't much of an argument. The Rails contributing guidelines discourage using and/or in favor of &&/|| -- why is this? Not due to any limitation or requirement of the language, but because it's been commonly decided that and/or have confusing semantics.

Returning truthy/falsy values from predicates rather than true/false is rude. It frequently clutters up IRB/pry/ruby-debug output.

It's also unhygienic. It risks breaking encapsulation. Consider this:

def party_time?
  @drinks && @emcee
end

This leaks an instance variable which may be a private implementation detail of the class. You have no idea where that return value might end up being stored, considering Ruby's implicit return. You might end up having that object stick around much longer than it is needed, because it's stored somewhere far away from that original method call. So we clean it up:

def party_time?
  (@drinks && @emcee).present?
end

So, what, we do this when it's necessary for encapsulation, and not at other times? Inconsistent and easy to apply incorrectly. It's much easier to establish a convention of returning true/false from predicates. It gives your objects a more consistent interface. It protects your implementation details. And it's friendlier to people interacting with the object in a console, or logging the results of that predicate.

!!value has been derided as cargo-cult programming in the past, but it seems to me that at this point not returning true/false from predicates has become a cargo cult itself. Please reconsider.

[clifton]

@fxn

There is a great argument to be had that my pull's ugliness, using !!(whatever) isn't worth it. I understand that.

You, and the other rails-core guys, are leaders in our field and owners of a fantastic web framework, many including myself would say it's the best web framework. Part of being a great leader is realizing when you're wrong. It's natural to be defensive and it's fine to argue every counterpoint. And you're right about many of them. But the strength of your argument is not stronger than the collective opinion of this community.

[fxn]

@clifton Nice sentence, except your code is the one who is broken.

Why? Because I am the one who defined the contract. Not you! The contract does not guarantee any singleton. And if the contract guarantees no singleton and the code expects a singleton, then the code is wrong.

Being defensive is an attitude, explaining the rationale and how predicates work in Ruby a hundred times is being patient. You could argue you are the one being defensive and stubborn and not admitting from the very moment you saw that line of code that the code had a bug. This thread had never had to start.

[clifton]

It started because you made a change 0aa66f0 which broke our [very large] application during an upgrade. I was upset. So I made a pull request as a wtf? knee-jerk reaction. The exact way I went about all of this was likely in bad taste.

That's not to say I wasn't right. In fact, I'd say I was because @tpope's pull request was merged in over your code as a result.

Don't get me wrong, I love Rails. I helped get a company off the ground (while I was in college without salary) and we have almost 40 employees now. And it's largely in part to a framework you and others contribute to. The work you've done has put a roof over the heads of all those people. That's pretty magical.

I'd just appreciate if you valued the opinions of others that have been working for years with your framework more. That doesn't make us right all the time, and I could be completely wrong on this issue too, but you've responded in a pretty negative way and written off some really great arguments without addressing them.

[fxn]

@clifton I think you can't deny I have tried to explain the rationale many times, and that I have said also many times that I respect the other point of view. If you read the thread top-bottom I think I have been always respectful.

But people assume that not applying a pull request is being defensive, or not listening to your users.

What has happened here is that I have explained very patiently and respectfully why the predicate was written that way. And I have no doubt about what I did.

You may differ, and that's fine. Even when I closed this PR I said some people may think different, and that's fine. But I am totally certain that the patch was correct and breaking no contract. That's being sure about one's rationale behind a code change, which is different from listening or not listening to users.

[asterite]

@fxn In all the example you gave about Ruby's standard library having predicates returning things that are not the singletons true or false, the documentation explicitly says what is returned. In the case of nonzero? it's "Returns self if num is not zero, nil otherwise. This behavior is useful when chaining comparisons". Other people showed you the documentation for other predicates.

Saying something returns true makes the user of that method expect it to be the singleton true regardless of the font you use.

With this change you are breaking someone's code (and possibly other people's code) and having no benefit at all except "This is Ruby style and I don't care what I said in the documentation". Also: why was the method changed? What was the improvement?

You have two choices: 1. make the method return true or false or 2. document that the method returns zero when the predicate is truthy. But I'd suggest doing 1.

[vertis]

@asterite while I generally agree with your points, indicating that @fxn has to do anything is incorrect. In fact I'm beginning to believe that he shouldn't do anything.

@fxn is (and has been) keeping sight of the bigger picture -- if you start making tiny changes based on the loud voices of a minority, then the code base will flip flop backwards and forwards any time someone has an issue (and sends it out to twitter). That's not a state we want rails to be in.

I'm not saying I agree with the project policy regarding predicates, because I don't. But you have to respect that there is one, and that this isn't the place to change that. If the change is impossible for @clifton to live without in his app, then he can create a monkey patch.

[konklone]

@vertis I guess I can respect your general sentiment, but I don't think there's any evidence that the people here are a "loud minority". There's been no polling on the issue, so if the overwhelming sentiment on this thread favors one thing, there's no inherent reason to believe they're a minority.

[vertis]

@klondike I think trusting the sentiment on a single thread, when blood has run hot, is not necessarily a good thing. The reality is, we're all wasting an inordinate amount of time on what is, as @josevalim pointed out, micro-optimization.

[Alamoz]

@vertis Changing the behaviour to work differently than before and differently than documented is flip flopping. If you feel strongly about leaving it alone, then why not change the documentation to specify that any value can be returned?

But do expect people to get pissed about lack of backward compatibility. Hopefully such changes will settle down moving forward.

[clifton]

I honestly did not care that my change was rejected (the patch in our app was obviously !!request.xhr?).

There are two things troubling to me here:

• @tpope's patch was reverted (which was nice, performant, and kept compatibility with Rails 2.3)
• Update ActionDispatch::Request#xml_http_request? documentation to match its behavior regarding return values #5572 was closed by @fxn

It's obvious there was no contract to return a singleton here. You're just screwing over people using a debugger by making them jump through a mental hoop and breaking compatibility for the sake of change.

I'll go back to work now. There are more important things to do.

[agraves]

The fact that this method returned 0 instead of true just cost us an hour of head-scratching.

I understand and totally respect @fxn's points about why 0 is correct, but I see no reason why of all the "truthy" values, 0 was chosen, except for the fact that the implementation happened to make it convenient.

I think the general delineation that fxn points to between truthy and untruthy values is quite wise...I can see how the behavior of .nonzero? benefits from essentially overloading the return value with additional information.

The question that clinches it for me, though is: what information are we allowing to bleed through by not casting this down to a boolean? The answer is: where a particular phrase is found within a HTTP header. I cannot think of a situation where a good programmer would leverage that.

I've always found it important in designing code for other coders to look beyond what is correct towards what is intuitive. The fact that a ton of really intelligent people have found their way here again and again is, to me, a very clear indication that the current behavior, while correct, is suboptimal.

And by the way, the code that distinguishes truthy values from true? The :layout option on Rails' own render...

[miry]

For those who want to see the true or false, I have published a monkey patching gem bang_bang_xhr.

[fxn]

Man you would need to patch the majority of predicates in Rails, the return value is irrelevant, what you can rely on is the documented return value, and we document predicates in general (there are exceptions) without committing to the exact object being returned.

This was part of the original discussion, this is not a rarity of xhr?, it is the way Rails defines predicates at the project level.

[jeroenhouben]

Superconfusing. And therefore should be properly documented. Right now the docs says:

Returns true if the “X-Requested-With” header contains “XMLHttpRequest” (case-insensitive). All major JavaScript libraries send this header with every Ajax request.

[ChristianPeters]

Well, I also fell into this trap as I was unaware of the fact that 0 is truthy in Ruby.

I think this is like a court decision that the public regards as counter-intuitive. Yes, it's formally right from a language perspective but it also violates POLA in a world dominated by languages considering 0 as falsey.

In my opinion, Rails should make a developer's life easier with easy-to-use APIs.

A first step into the right direction would be to talk about "truthy values" vs. true in documentation. Who expects the non-existence of a special formatting (fixed width font) to be relevant before reading through an issue like this?