Skip to content
This repository

Remove "needless boolean casting" #5582

Merged
merged 1 commit into from about 2 years ago

14 participants

Jack Chen Odin Dutton Richo Healey Michal Papis Ben Woosley Yi-Ting Cheng Julian Doherty Xavier Noria Ary Borenszweig Eugene Diachkin David Celis Jake Palmer Ian Leitch Eloy Durán
Jack Chen

"Predicates in Rails rely on standard Ruby semantics for boolean values
and guarantee no singletons whatsoever." - @fxn

Jack Chen Remove 'needless boolean casting'.
"Predicates in Rails rely on standard Ruby semantics for boolean values
and guarantee no singletons whatsoever." - @fxn
ef64c6b
Jack Chen

Related to 3756a3f

Richo Healey
richo commented March 25, 2012

+1 arbitrarily changes core behaviour for no obvious reason.

Ben Woosley

Correction, @richoH, there's a clear performance benefit, and less code is better code.

+1

Yi-Ting Cheng
xdite commented March 25, 2012

+1

Julian Doherty madlep commented on the diff March 26, 2012
activerecord/lib/active_record/relation/finder_methods.rb
@@ -200,7 +200,7 @@ def exists?(id = false)
200 200
         relation = relation.where(table[primary_key].eq(id)) if id
201 201
       end
202 202
 
203  
-      connection.select_value(relation, "#{name} Exists", relation.bind_values) ? true : false
  203
+      connection.select_value(relation, "#{name} Exists", relation.bind_values)
1
Julian Doherty
madlep added a note March 26, 2012

misunderestimated level of truthiness

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Xavier Noria
Owner
fxn commented March 26, 2012

This proposal makes the implementation follow our policy for predicates. The method is documented as a generic Ruby predicate, and we are heading a major 4.0. So everything is good. Thanks for the pull request.

Xavier Noria fxn merged commit ded74df into from March 26, 2012
Xavier Noria fxn closed this March 26, 2012
Richo Healey
richo commented March 26, 2012

... seriously?

Jack Chen

Where is this "policy for predicates"?

Ben Woosley

Incidentally @fxn, I don't have a problem with this one, as the result has a clear, articulable, documentable logic to it. #xml_http_request?, not so much.

Ary Borenszweig

Thanks, I've been waiting for this since 2.3

Jack Chen
Xavier Noria
Owner
fxn commented March 27, 2012

It has been merged because of the reasons explained in the comment above.

Please this is much more simple than you imagine.

I have no point to prove. This PR has been applied because of the exact same reasons the other PR has been rejected.

Disabling notifications from this one as well, too much going on.

Eugene Diachkin
ineu commented April 19, 2012

There is another proposal: remove ALL predicate methods from Rails and from all ruby code including stdlib. All ruby methods return something, and every value can be converted to boolean by !!, so every method returns truthy or falsy value. It won't be any less absurd then this one.

David Celis

That's a joke, right?

Eugene Diachkin
ineu commented April 19, 2012

@davidcelis I don't know. It could have been joke if @fxn didn't think that predicates can return anything at all that is 'truthy' or 'falsy' instead of real booleans. This very commit was meant to focus on how illogical this is, but as you see the commit is merged and now we have predicates that return id. Is this a joke?

Jake Palmer

yes

Jack Chen

Well the goal of my pull request was to prove how stupid the whole idea is. Just need to get someone other than fxn to look at it. There's quite a lot of places where Rails predicate methods return real true and false. I don't see how making them not return true or false is sane.

Xavier Noria
Owner
fxn commented April 19, 2012

The way we look at predicates in the project is: write natural code, and document a generic contract in any case. Natural code for us does not include double negations or ternaries or that kind of forcing a singleton. If the natural code returns a singleton, fine. If it does not, fine. You cannot rely on a singleton in any case if the documentation does not guarantee one, so it just does not matter to your code, client code is not going to be able to rely on it in any case.

If you wanted to prove a point and you qualified that policy as "stupid"... what can I say man. I don't care. Your commit fit in this guideline, as I explained in the rationale, so it went it.

That said, I had unsubscribed from all the different discussions about this thing, and didn't from this one. I am going to do that now.

Jack Chen

The problem that most people have is that as users of the Rails API, they have issues with predicate methods returning truthy values when they could be proper boolean values. Nobody likes an API that doesn't do things that people expect, even if the documentation qualifies it to do so.

Jack Chen

Just because the documentation says so doesn't mean it's law and must be correct. Think of this as the pro-gay marriage protests.

Ian Leitch

The whole "truthyness" reasoning for accepting these kind of patches is kinda valid. However I see no benefit, only a new situation where some poor developer is going to encounter unexpected behaviour. Isn't one of the original design goals of Ruby for it to "do what you expect"?

if some_method? == true

This is an uncommon idiom in Ruby, but many people are new to Ruby, come from another language or just want to be explicit for some reason.

Would you expect a method ending in a question mark to return true or false?. I'm sure 99% of developers would.

Eloy Durán
alloy commented April 19, 2012

@chendo

Of course it was merged merely to prove a point. I have a feeling if anyone else saw this pull request, it would be rejected.

You were trying to make a point, by creating a pull-request for something that you happen to find incorrect, but of which @fxn has been consistently saying the same thing, namely that that’s how it’s done in Rails. And what did he do? He merged it, because he was being consistent, again.

Do you really think other core members don’t know of this patch? Face it, it hasn’t been rejected because nobody else saw it. Everybody in the Rails community has seen this shitstorm.

Nobody likes an API that doesn't do things that people expect.

Read all posts from @fxn from the very beginning. What people should expect with Ruby is to not expect the true and false singletons from predicate methods. It really is that simple.

Just because the documentation says so doesn't mean it's law and must be correct. Think of this as the pro-gay marriage protests.

This is absolutely nothing like pro-gay marriage protests. Your opinion != the same as basic human rights, please don’t compare such an important topic with a ‘minor’ issue like this.

I’m sure you’re doing this all because you care, but there is no point in arguing over it anymore. It is not going to change. The only thing that all this does is take away more time that @fxn and other core members (and you) could have spent on stuff that is actually open for discussion.

Just remember, this is not a democracy.

Jack Chen

The entire reason that caused this massive shitstorm in the first place is that it's reverting a commit that gave most people expected behaviour in the first place. Just because the documentation says so does not mean that it's behaviour people expect.

If someone specced an API function named "does_x" but the documentation states that it does not actually do x, I don't think people would be okay with that.

If it's basic human rights we're talking about, then why isn't it already legal? Why are there protests around it? It's good to know that Rails is not that different to real life politics.

Eugene Diachkin
ineu commented April 19, 2012

@alloy

Read all posts from @fxn from the very beginning.

Yes, I read all of it. The main idea is "this is my opinion and the only authorities for me are myself and Matz, not you, "dudes".

What people should expect with Ruby is to not expect the true and false singletons from predicate methods.

Well, it differs from what people do expect. Most predicates return booleans in ruby and rails. People got used to it. One should not do something different without really good reason. And sorry, but there's no good reason here. It really looks like wise fxn decided to teach little silly kids how to use ruby predicates the right way.

Eloy Durán
alloy commented April 20, 2012

@chendo

The entire reason that caused this massive shitstorm in the first place is that it's reverting a commit that gave most people expected behaviour in the first place. Just because the documentation says so does not mean that it's behaviour people expect.

I’m pretty sure people will be able to deal with it, though.

If someone specced an API function named "does_x" but the documentation states that it does not actually do x, I don't think people would be okay with that.

I get your point and why you make the comparison, but it’s not the same in this case, seen from a different point of view. A Ruby predicate method that does not return true or false still works, as long as truthy/falsey (is that’s what a non-truthy values is called??) values is returned.

If it's basic human rights we're talking about, then why isn't it already legal? Why are there protests around it? It's good to know that Rails is not that different to real life politics.

It is legal where I’m from :)

But, wether or not it is or isn’t legal in some countries does not make it any less of a human rights issue. This issue on the other hand is about opinion. Sometimes that means we have to agree to disagree, but in such a case the owners of a project will have to make the final call.

To be clear, I have disagreed with many things in Rails in the past and probably will in the future, but a project like Rails simply can’t make progress by not taking an opinionated stance and saying ‘no’ sometimes.

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

Showing 1 unique commit by 1 author.

Mar 26, 2012
Jack Chen Remove 'needless boolean casting'.
"Predicates in Rails rely on standard Ruby semantics for boolean values
and guarantee no singletons whatsoever." - @fxn
ef64c6b
This page is out of date. Refresh to see the latest.
2  activerecord/lib/active_record/relation/finder_methods.rb
@@ -200,7 +200,7 @@ def exists?(id = false)
200 200
         relation = relation.where(table[primary_key].eq(id)) if id
201 201
       end
202 202
 
203  
-      connection.select_value(relation, "#{name} Exists", relation.bind_values) ? true : false
  203
+      connection.select_value(relation, "#{name} Exists", relation.bind_values)
204 204
     end
205 205
 
206 206
     protected
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.