Skip to content
This repository

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

Closed
wants to merge 0 commits into from
Clifton King

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");
Xavier Noria
Owner
fxn commented

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 King

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.

Xavier Noria
Owner
fxn commented

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 King

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.

Xavier Noria
Owner
fxn commented

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.

Xavier Noria fxn closed this
Claudio Poli

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

Xavier Noria
Owner
fxn commented

@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.

Claudio Poli

@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 King
# 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.

Xavier Noria
Owner
fxn commented

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".

Tony Arcieri

"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.

Xavier Noria
Owner
fxn commented

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.

Peter 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.

Tony Arcieri

@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.

Xavier Noria
Owner
fxn commented

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.

Luke Chadwick

@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.

Tony Arcieri

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

Clifton King

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?

Kyle Neath

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 King

thank you @kneath :clap::clap::clap::clap::clap:

Tony Arcieri

+1 there @kneath

Xavier Noria
Owner
fxn commented

@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.

Jesse Brown

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.

Adam Prescott

@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?

Xavier Noria
Owner
fxn commented

@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?

Xavier Noria
Owner
fxn commented

@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).

Tony Arcieri

:trollface::trollface::trollface:

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.

Matthew Boeh
mboeh commented

"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.

Adam Prescott

@fxn going to have to echo the sentiment that, to someone familiar with Ruby, documentation saying "returns true" does not mean "returns truthy object", and so the contract was altered by 0aa66f0.

Luke Chadwick

@fxn I disagree. Just saying you use a different font for singletons, doesn't change how everyone would read that.

It's a well accepted convention that methods with a ? will return a true or false boolean value, not just work in an if statement.

There are many examples of the following:

def value
   "Something"
end

def value?
   !!value
end

In the example you're suggesting, the later method should never exist, because I can quite easily do:

if value

And it will just work.

I'm with @kneath on this anyway, what's the problem? Will it cause a performance issue? The 2.3 method is more flexible, and restoring it will prevent confusion when people upgrade.

Xavier Noria
Owner
fxn commented

@aprescott I disagree. Any predicate is documented that way. It is a natural way to express the behavior of a predicate isn't it.

That is also documented http://guides.rubyonrails.org/api_documentation_guidelines.html#regular-font

But users may or may not know the guidelines. Ruby programmers know a predicate returns something to be interpreted in a boolean context. You cannot expect the singletons! The singletons are for the author of the predicate, not for the caller to expect them.

Personal preferences are one thing, a formal language is a different thing. Client code is client code.

Kristopher Murata

I really don't see the point on this discussion, it's just a matter of a consistent pattern within Rails regarding methods with the predicate "?". Which is a best practice in any language/app.

Do Rails have all ? methods returning true or false? Does this breaks the pattern? If so, this is a no brainer. But I really doubt that this is the case.

On another hand, I personally I agree that returning true or false is more straight forward and the expected behavior, and this PR doesn't break anything, why not accept it?

Sometimes it feels like core team is always highly defensive about core rails code. :trollface:

Tony Arcieri

@fxn The least confusing symbol you can use for true is true. It seems you're defending being intentionally confusing

Xavier Noria
Owner
fxn commented

@vertis

It's a well accepted convention that methods with a ? will return a true or false boolean value, not just work in an if statement.

No, it is not a well accepted convention. That's why Ruby does not require that you return the singletons and define what is true and false. Because you can use that.

File.world_readable?("/etc/passwd") # => 420
File.world_readable?("garbage")  # => nil

Look! Standard library, no singleton.

People, you cannot rely on the values of predicates unless documented. Predicates are to be interpreted in boolean context.

Clifton King

@mboeh your post mirrors my sentiments exactly. I used the (@one && @two).present? example right above yours. Thanks for putting the argument into better words.

@bionicpill that JS example was contrived, and the problem was actually leaking the implementation detail to another piece of Ruby and not JavaScript.

Xavier Noria
Owner
fxn commented

@krsmurata because there is nothing wrong about the semantics. And while I respect people may have other stylistic preferences, I write

 def has_name?
   first_name && last_name
 end

which is short and idiomatic.

So it is not a matter of being defensive, it is that this is my code base. I am not going to go to your code base to tell you what your code style you should use because I expect it due to my preferences.

Adam Prescott

@fxn well, to draw a fair comparison, the documentation for File#world_writeable? doesn't make claims about returning true.

If file_name is writable by others, returns an integer representing the file permission bits of file_name. Returns nil otherwise. The meaning of the bits is platform dependent; on Unix systems, see stat(2).

Even if you accept "you can't rely on predicate method values to be true or false", the documentation between the two examples differ. If the implication in the Rails guidelines you linked to is that foo? can be described as returning "true" while not returning true, then I simply disagree with the guidelines, I suppose.

Tony Arcieri

@fxn can you at least confirm you think doing the more confusing thing here is actually a good idea? And perhaps you can actually tell us why you think it's a good idea, as opposed to just cargo culting the Ruby standard library, which, IMO, is not exactly the paragon of what I'd consider good Ruby code

Richard Livsey

I get the argument that client code shouldn't expect / rely on the singletons being returned, but I fail to see how the downsides weigh against accepting this request.

As far as I can tell:

Cons for returning singletons:

  • one/two extra characters in the codebase (multiplied by number of predicate methods obviously)
  • maybe lulls developers into a false sense of security that all predicate methods everywhere return singletons?
  • ...?

Cons for not returning singletons:

  • confusing for consumers of the library, even those who have read the documentation style-guides
  • leaks encapsulation
  • messes up logs, forces all client code to coerce to booleans first
  • messes up output in IRB/debugger

For a framework which added in #second, #third etc... to Array I don't see extra characters of code being a good argument against accepting this, so it must be a question of purity.

Rails famously "optimises for developer happiness", I'd have thought purity should take a close second place to obviousness where reasonable?

My 2¢, worth considerably less than that!

Xavier Noria
Owner
fxn commented

@tarcieri I have expressed the rationale enough in the whole thread I believe.

Tony Arcieri

@fxn I don't think you've touched on the issue of developer confusion at all, except perhaps trolling those that would be confused by using 0 to mean true

Xavier Noria
Owner
fxn commented

The reason this PR is not accepted is clear. Double negations are unnecessary, the code is more clear without them. And that is why it was changed. To be idiomatic and do not force singletons when they are unnecessary.

Some people would write that method in a different way. I totally respect that.

collin

@fxn You/the rails documentation are doing a really great impression of Bill Clinton.

"It depends upon what the meaning of the word 'is' is. If the—if he—if 'is' means is and never has been, that is not—that is one thing. If it means there is none, that was a completely true statement"

"regular font" true vs true is the least communicable idea I've heard in months. And this is GOP primary season.

Tony Arcieri

@fxn I'm actually not in favor of this pull request, however I would be in favor of reverting 0aa66f0

What value does it actually add? You eliminate one boolean inverse operation? It seems like you're microoptimizing this method at the cost of confusion. Before it returned true/false. Now it returns 0/nil. true/false is a lot less confusing than 0/nil.

Adam Prescott

Is !!(a =~ b) really that much more confusing to a Ruby programmer compared to a =~ b that it warrants keeping it as the latter for this one-line method?

Brian Schroeder
bts commented

if i was playing around with a library and it returned 0 from some_method? i would immediately assume there's probably a bug in the code

Xavier Noria
Owner
fxn commented

@tarcieri have you read the thread? in no place I've said optimization was a motivation. Motivation is to simplify the code leveraging the fact that using a boolean operator as such is valid and idiomatic.

Matthew Boeh
mboeh commented

@fxn It's not "code style" when the change bubbles up to client code. You haven't bothered to address the encapsulation, logging, or IRB arguments I and others have offered. Instead, you're persisting in treating the semantics as irrelevant. You can't justify yourself by claiming it's a matter of style while simultaneously ignoring counterarguments to that claim.

Xavier Noria
Owner
fxn commented

@mboeh because those are not counterarguments at all. You are surprised that a predicate prints nil in IRB. Fantastic, that means you need to revise your expectations about the semantics of Ruby!

Clifton King

@tarcieri I'd also be in agreement with reverting 0aa66f0

I should have created the pull request with that instead.

The kind of refactoring that's awesome for rails is the kind of optimization that @tenderlove :heart::heart::heart: has been doing. Breaking API compatibility for a tiny amount of CPU cycles in a language that's optimized for programmer happiness rather than performance is the kind of optimization that wasn't worth 0aa66f0 in the first place.

And saying the compatibility wasn't in the contract because the word true wasn't in a fixed-width font is pretty mind-boggling when the documentation clearly says it returns true.

I guess we just have differing opinions.

Thanks for all your great work on Rails, but I hope you can really think about this kind of issue and take a more sensible approach in the future.

Xavier Noria
Owner
fxn commented

@clifton as I have said before, the motivation are not CPU cycles. Performance has not been mentioned.

Peter Wagenet

@fxn, so !! may not be the correct solution to this, but it still seems like a problem. Just because returning 0 is a valid response, doesn't mean it's ideal. As others have pointed out, Ruby allows certain things that are not ideal. That doesn't mean we should do them. It seems like you are holding your ground on this merely because technically it is ok, even if it is not actually the clearest thing to do. If your response was "yes, this is confusing, but !! is no better" then I would understand and I would hope that together we could all find a better solution. I think we need to keep the principle of least surprise in mind here.

Matthew Boeh
mboeh commented

@fxn If you're going to misrepresent one of my arguments and ignore the rest, you could do the courtesy of not being condescending about it.

collin

@wagenet least surprise indeed.

Did a quick survey of built-in predicate methods. They return true and false.

I would expect the same here.

Tony Arcieri

@fxn check my comments on 0aa66f0, your change actually caused a performance regression on JRuby and Rubinius.

I would strongly be in favor of reverting it, not for the performance impact, but for the more unclear return values. The performance regression is just icing on the cake. There is absolutely no benefit in that change.

Eloy Durán
alloy commented

@bts and that’s the problem. It should be made clear that that’s not something you should assume in Ruby. @fxn has imo more than enough explained why in general this should not be expected behavior.

Choosing between a truthy value vs true is a style-issue in Ruby. This is good, because it allows you to return values other than true or false when you need/want that, but still have a nice ? at the end of the method. Changing this concept would mean we as lib authors lose freedom, as users will start to expect it more and more.

So imo, even-though I understand the OP’s point on this specific case, in general you can’t expect people to change their style choice when you want to use their code (Rails).

Adam Prescott

Following on from what @collin said, for what it's worth, the guideline content on fixed-width vs regular fonts for true vs "true" doesn't appear to have been changed in quite a while:

https://github.lifo/docrails/blame/master/guides/source/api_documentation_guidelines.textile#LID131

Would changing the guidelines to make "true" == true be too far-reaching in terms of the return values for existing predicate methods in Rails? In other words, do the vast majority of Rails predicate methods rely on this guideline?

Luke Chadwick

@alloy that's just the thing though. You can have an expectation that the behaviour of a public method shouldn't change without a good reason. I do not find removing a couple of ! to be a good reason.

I've learned something today, about my preconceived notion that methods with ? will always be true or false. I'll digest that later.

The reality is in this case there is no harm in going by going back to the previous implementation, and a lot of potential harm and confusion by keeping it the way it is now.

Tim Pope
tpope commented

I hate to dog-pile on a thread like this, but, well, I almost feel obligated. It boggles my mind that anyone would defend this behavior. I don't mind that it's backwards incompatible. I don't mind that I can't do request.xhr? == true. But to justify mindfucking anyone who dares to invoke request.xhr? in a debugger with "the docs don't say it won't do that" is just plain silliness.

If double negation is such a deal breaker, there are plenty of alternative implementations. For example:

/XMLHttpRequest/i === @env['HTTP_X_REQUESTED_WITH']
Luke Chadwick

@tpope brilliant.

Peter Wagenet

@alloy, it seems to me that Rails has grown beyond the point of being just "someone's code". Obviously, the core team is the final arbiter of any decisions, but it's also important to listen to the concerns of the actual users. This is especially true in cases like this where there is no compelling argument to return a non-boolean value other than "Ruby allows us to". If users find something confusing and there is no noticeable loss in making it less confusing, then why not make the users happy?

Piotr Sarnacki
Collaborator

While I don't usually rely on methods with ? to return boolean value, I don't see any point in making it harder to rails devs. In most cases you don't care, but xhr? is somehow related to javascript (for example it's sometimes used in JSON responses).

@fxn while I understand your point that people should not rely on such behavior, can we be pragmatic and just return what's expected here? The documentation stated that it returns true and while we may argue if it means only truthiness or actual true boolean value, a lot of people understand it as the latter (including me).

Eloy Durán
alloy commented

@vertis I know, that’s why I said that in this specific case I understand the OP’s issue with it.

For what it’s worth, if this were my codebase, I would revert it to what it was. Simply because I returned a value in the past (true or false) that could easily have been interpreted as meaning that the singletons can be expected.

I would not, however, go through all other predicate methods and change them to return true or false if they did not do that already.

Matthew Boeh
mboeh commented

@alloy When you're providing a public API thousands of people use every day, "freedom" isn't something you get anymore.

@tpope +1. I'd like to see this in a pull request, in order to find out whether double negation is really the problem here.

Tony Arcieri

@tpope +1 for your version

Eloy Durán
alloy commented

@mboeh Wow. That’s not a very nice and motivating thing to say.

Eloy Durán
alloy commented

@tpope I like that, learned something today. Thanks :)

Eric Mill

I've been coding in Ruby for almost 6 years now, @fxn, and I suspect many others on this thread each have several years of experience. You're defending your argument as "the semantics of Ruby". If just about every Rubyist on this thread is disagreeing with you about what "the semantics of Ruby" are, you should think about the possibility that you are wrong.

A method ending with a question mark returns true or false, unless there's a specific and good reason to make it truthy instead. If you don't have one here (and it seems to have worked this way just fine in 2.3), then you should change it to return a boolean.

Tim Pope
tpope commented

Pull requested. We shall see.

Xavier Noria
Owner
fxn commented

@klondike please refer to Flanagan & Matz above. That's what says how Ruby predicates behave, not a set of Ruby programmers that would like it to be different than how it is (which as I have said several times here I totally respect).

Kristopher Murata

@fxn wait wait, I'm not accusing the core team of anything, far from me pointing fingers when I do nearly nothing for the rails goodness these days. Maybe I should use #joke tag instead of a trollface. :)

The only thing that concerns me is changing the public API in favor of removing double negation form the code. In my opinion any change on the Rails public API is a big deal and should be avoided if necessary. Even if it's a matter of 0 or true.

Tony Arcieri

It's all moot now ;)

Alex Sharp

Call me crazy, but if people who wish to contribute to, or read, rails' source code, can't be bothered to perform the mental hurdles to understand a !! in ruby code (which, imo, is relatively idiomatic in and of itself), then we've got bigger problems. But that's not what this thread is really about.

This thread devolved into a largely theoretical argument about what predicate methods in ruby code should/shouldn't do, and whether theoretical code should/shouldn't expect certain values from methods, and method contracts, encapsulation, etc. All good arguments that never should have had to be made. This should have been a trivial commit, but for some reason we're still talking about it. There are more important problems to solve than this. :sadpanda:

Still, I'd be remiss if I didn't color my criticism of rails core with a great deal of thanks for the hard work you do. So thanks!

Ben Bleything

@fxn: your handling of this situation is very disappointing. You've had a user complain that a change made things more confusing, and your response is, effectively, "too bad, that's how it works".

There's no technical argument to be made for that method returning a boolean, but there's also no technical argument to be made for it returning anything else. Given that's the case, why not err on the side of easier to use?

@tarcieri asked you directly, twice, to address this point, but you haven't done so. Could you please give us a rationale for making a change that increases confusion with no other benefit?

collin

@fxn What is the reason for xml_http_request? to have an a-typical response?

collin

Oh sweet, awesome pull request won. Adios.

Xavier Noria
Owner
fxn commented

@collin the response is not atypical. The predicate returns a boolean value.

collin

@fxn "Predicates typically return one of the Boolean values true or false" is not the same as "Predicates typically return one of the Boolean values true or false or 0"

Xavier Noria
Owner
fxn commented

To all in this thread.

The PR that has been applied does not change the API contract: We are not guaranteeing in any way that xhr? is going to return singletons next year. And in general no Rails predicate guarantees that.

So, if after this thread you still rely on expected singletons you have a time bomb.

Clifton King

@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.

Xavier Noria
Owner
fxn commented

@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 King

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.

Xavier Noria
Owner
fxn commented

@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.

Xavier Noria fxn referenced this pull request from a commit
Xavier Noria Revert "Return an actual boolean from xml_http_request?"
Reason: This commit changes code that was committed some year
and a half ago. The original code is an ordinary predicate
that delegates straight to a boolean operator with no further
unnecessaru adorments, as clearly explained in #5329.

This change also may confuse users who may now believe they can
rely now on singletons, while predicates in Rails rely on
standard Ruby semantics for boolean values and guarantee no
singletons whatsover.

This reverts commit 6349791.
3756a3f
Ary Borenszweig

@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.

Luke Chadwick

@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.

Eric Mill

@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.

Luke Chadwick

@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.

Adrien Lamothe

@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 King

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)
  • #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.

Aaron Graves

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...

Michael Nikitochkin
miry commented

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

Xavier Noria
Owner
fxn commented

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.

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

Sorry, commit information is not available for this pull request.

This page is out of date. Refresh to see the latest.

Sorry, Diff contents are not available for this pull request.

Something went wrong with that request. Please try again.