Skip to content
This repository
Browse code

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.
  • Loading branch information...
commit 3756a3fdfe8d339a53bf347487342f93fd9e1edb 1 parent 014498e
Xavier Noria authored March 25, 2012
2  actionpack/lib/action_dispatch/http/request.rb
@@ -154,7 +154,7 @@ def content_length
154 154
     # (case-insensitive). All major JavaScript libraries send this header with
155 155
     # every Ajax request.
156 156
     def xml_http_request?
157  
-       /XMLHttpRequest/i === @env['HTTP_X_REQUESTED_WITH']
  157
+      @env['HTTP_X_REQUESTED_WITH'] =~ /XMLHttpRequest/i
158 158
     end
159 159
     alias :xhr? :xml_http_request?
160 160
 

76 notes on commit 3756a3f

Ben Woosley

I'm -1 on this - "?" methods can return non-boolean results, but the only legitimate reason to do so is to return additional information beyond true or false. When you simply return 0 for true and nil for false, you add an additional layer of indirection from the simplest representation of the return value, without any redeeming additional information.

Sure, "?" methods can return non-bool values, but this is just sloppy, IMO.

Steven Hancock

I'm also -1 on this. Regardless of whether or not a ? predicate method can return values other than true or false, in this case there is no good reason other than "because I can."

In addition to that, if you want to talk about Ruby semantics, Request#xhr? is literally asking "Is this an XMLHttpRequest?" so the code /XMLHttpRequest/i === @env['HTTP_X_REQUESTED_WITH'] which literally asks "Does this match?" is more semantically correct than the current code which literally asks "Where does this match?". "Where does this match?" is not the question we're asking here when we call Request#xhr?, is it?

Kenneth

+1. This makes sense.

Steven Hancock

@kentor how does it make sense? Realistically it doesn't make a difference as long as you're returning a truthy or falsy value, of course, but how can anyone say it's semantically correct to change the code to ask the wrong question? If I want to know "Does this match?" I ask "Does this match?" and not "Where does this match?", right?

There is no performance impact either way (actually, this hurts performance slightly in some Ruby implementations compared to the use of !! that this originally replaced), the function returns a truthy/falsy value either way, so what does it hurt to actually be semantically correct instead of asking the wrong question and saying "It's ok because Ruby lets me do it."?

Kenneth

If I really wanted to ask "where does this match?", I would use String#index. String#=~ I would use in a boolean context.

Steven Hancock

String#=~ returns nil if there is no match, or the index into the string (or, where the match occurs) if there is a match. That's clearly a question of "Where does this match?" or, at least, "Does this match? If so, where?"

We don't need or want the answer to "If so, where?"

Aside from asking the wrong question (or maybe, asking the second part of question when it isn't necessary), as documented by @tarcieri in the discussion at commit 0aa66f0 this introduces a performance degradation in at least two popular implementations of the Ruby language (JRuby and Rubinius). Since either way is "technically" correct, asking the right question and returning an actual boolean value is more semantically correct, and it's been clearly demonstrated that not returning a boolean value can have a negative impact on performance.. there is no good reason not to return a boolean value. "Because I can" isn't a good enough reason, even "Because Matz says I can" isn't a good enough reason.

Xavier Noria
Owner
fxn commented on 3756a3f March 25, 2012

@stevenh512 =~ is a boolean operator.

It returns an offset in case it is useful for you not because it assumes you want it. So, people write

if string =~ regexp
  ...
end

routinely. Semantically is perfect, I disagree with you in that respect. You think semantically is not adequate because you think true is semantically more adequate than 0. Nope, not according to Ruby semantics of what is true and what is false (regular font).

That said, given both implementations, the one based on =~ and the one based on ===. They are both correct and I am OK with the two of them. What I disagree with is with having this commit as a reaction to some people that want a singleton, as I explain in the rationale.

They could understand (and I think some did), that this commit means singletons win, and that they can rely on singletons. And that is something my revert wants to prevent. We are not changing the contract in any way, and your code cannot depend on the exact returned value, only in its boolean interpretation by Ruby.

It is a predicate, we only promise it acts as a predicate.

Steven Hancock

Semantics or not, it has been shown that this commit can and does introduce a performance degradation in at least two popular implementations of Ruby. If there's no other reason to not return an actual true or false other than "Because I can" or "Because otherwise someone might actually think it returns true or false" (not and unreasonable assumption, since most ? predicates do unless specifically documented otherwise), and you just said you believe that both are semantically correct, then why not go with the one that doesn't introduce a performance degradation?

As far as whether they actually are both semantically correct, and whether or not String#=~ is a Boolean operator, let's see what the Ruby docs (which are more of an authority on the subject that me, you, or anyone else who has commented on any of these discussions) says about String#~:

If obj is a Regexp, use it as a pattern to match against str, and returns the position the match starts, or nil if there is no match. (emphasis mine)

That's clearly and unarguably a question of "Where does this match?" which is not a true or false question, not "Does this match?" which is a true or false question.

Now let's ask the Ruby interpreter itself. Let's give the interpreter the following sentence: "This is a string of text." Then let's ask the interpreter "Does this sentence contain the word text?

$ irb
1.9.3p125 :001 > sentence = "This is a string of text."
 => "This is a string of text."
1.9.3p125 :002 > sentence =~ /text/
 => 20
1.9.3p125 :003 > /text/ =~ sentence
 => 20
1.9.3p125 :004 > /text/ === sentence
 => true

If you asked me "Does this sentence contain the word text?" and I answered 20 I would be wrong. Sure, in Ruby that is a truthy value and nil is a falsy value, but to argue that it's semantically correct goes against all common sense and the very definition (as pertaining to computers) of the word boolean.

You say both are semantically correct. I'm one of many people with varying levels of experience with Ruby who say === is more semantically correct. This commit has been shown to introduce a performance degradation. At least one member of the Rails core team (who, if I'm not mistaken, is also a member of the Ruby core team or at least a contributor) saw no problem with merging the pull request that you're reverting here.. and the only good reason is "Someone might think a boolean operation should return true or false?" I guess that's a better reason than "Because I can" or "Because Matz says I can" but I still don't think it's a good enough reason to introduce a performance degradation when returning actual true or false in reality hurts nothing.

Xavier Noria
Owner
fxn commented on 3756a3f March 25, 2012

First thing, =~ has been there for about a year and a half. When I pushed that code in 2010 it was removing unnecessary boolean conversions.

Second, the change to === is not motivated by performance, it has another goal related to the discussion in the aforementioned PR.

Third, =~ is a boolean operator, perfect to delegate to. If you'd prefer Ruby to be different with regard to booleans go argue with language designers. I am using Ruby, not the imaginary language you would like Ruby to be. Alternatively please go use Java, you'll get plenty of singletons there.

Fourth, are we really going to roflscale xhr?

Steven Hancock

I never said I'd prefer Ruby to be different with booleans, but by the very definition of the word boolean, =~ is not a boolean operator regardless of the fact that it's quite often used as one "because you can". I never said I wanted Ruby to be some other imaginary language, don't put words in my mouth, I quoted the docs and the interpreter directly to back up my opinion. There's no way you can possibly argue that === is not semantically correct (since you already said yourself that it is), regardless of the motivation of the pull request it was pulled in by someone who is both a Rails and Ruby contributor and core team member (so maybe I'm not the one who should be arguing with language designers here?).

As for your last question, I'm sure xhr? gets called quite often in many apps, especially with AJAX front-ends being more and more common these days, so if we can remain semantically correct and not take the slight performance hit, why not?

With that, I'm done with this discussion. You've already shown that you're going to do whatever you want anyway so there's no use arguing.

Xavier Noria
Owner
fxn commented on 3756a3f March 25, 2012

Let me put it in another way.

If @tpope had contributed a commit unrelated to the PR with commit message "improve performance of xhr", I would be all for it. It is also based on a boolean operator whose return value is irrelevant for the contract of xhr? as the one of =~ is.

But a commit as a reaction to the PR with commit message "return an actual boolean value". No thanks, there's no need to do that, and people cannot rely on that. They could not rely on it even if the commit was in, because the contract does not say so.

Steven Hancock

I'm on an extremely slow internet connection at the moment and it would literally take me 2 hours to pull down the entire Rails repo, but even still, I'm almost tempted to put that to the test. Afterall, the benchmarks have already been done, it's just a matter of updating my copy of the repo and sending the request. If nothing else it might at least get my name back in the commit logs (it's been quite a while, and I think it was a docrails patch last time). :grin:

I will agree with you on this much.. a ? predicate method can and sometimes does return a value other than literal true or false and because of that a developer should not rely on it returning a literal true or false. I don't think that means you should return something other than a literal true or false if there's no need to, especially not without clearly documenting that fact.

At the very least, regardless of any arguments of whether or not it's semantically correct, it violates the principle of least surprise and it has been demonstrated that xhr? can return true or false without the double-negation that you originally got rid of.. although I'm sure I'm not the only person who would argue that double-negation has been used often enough and for long enough that it's as much a Ruby idiom as using =~ as a boolean operation is, so if =~ is correct then double-negation was also correct and there was no reason to change it in the first place. Not to mention the fact that double-negation is also semantically correct in math (where "a negative times a negative equals a positive") and in the English language (where a double-negative is a positive) so it at least doesn't violate the principle of least surprise as much as returning a non-boolean value for no reason without properly documenting it (the docs say it returns true or false, regardless of the font used, it's not unreasonable for someone to expect it to return true or false and not "truthy" or "falsy".. especially newer developers trying to learn Rails and being caught by surprise before they know the documentation guidelines).

Steven Hancock

Actually, maybe an edit in docrails would be the best solution.. although it does hurt performance very slightly in some Ruby implantations and I could see it potentially being a small part of a performance problem in certain situations (I'll admit, there would probably have to be several other issues for it to be noticeable without an extreme amount of traffic).. my biggest concern is really the "principle of least surprise" and that can easily be fixed by documenting that it "returns a value that evaluates as true or false" or something to that effect rather than saying that it "returns true or false" (again, regardless of font, that could be surprising and confusing to new developers trying to learn Rails). :D

Ben Woosley

@stevenh512 - for the docrails edit, see my #5572, also closed by @fxn :-P

Steven Hancock

@Empact and that's part of the reason my name hasn't been in the Rails commit logs for quite some time (but it is there, pretty far back in the history by now, but CoderWall found it), I've found that a lot of other projects are much more reasonable about accepting proper documentation patches. :grin:

Although, personally, I would have said "returns a value if" rather than "returns true if" or "returns 0 if" in that particular instance (and maybe added "returns false or nil otherwise", or maybe not).. which is correct, makes sense and can not in any way be seen as "confusing,", "surprising," or "leaking implementation details" so any objection to clarifying it in that way would be nonsense.

Xavier Noria
Owner
fxn commented on 3756a3f March 25, 2012

What? Dude do you know what is docrails and the gazillions patches Rails (and personally me) has applied? What are you talking about?

The proposal to document a 0 is rejected, and the reject explains why. There is nothing else to add.

Andrés Mejía

I don't see why this commit had to be reverted. This pretty much feels like a decision taken by a dictator.

Tony Arcieri

-1. WTF.

I understood "it clears up the logic" when you changed this originally, even if it returned the worst truthy value you can possibly return. But why would you revert a version which is both clear and returns something much, much better than zero to mean the truth: true.

Tom Dale

-1. This just seems like petty stubbornness.

Richard Schneeman
Collaborator

-1

I prefer the code that was reverted, agree with @tarcieri

Jamie Gaskins

@tomdale So do the vast majority of the opposing arguments. The exact return value of this method isn't worth all this fuss.

Scott Gonyea

Wow, that's really shitty of you @fxn.

Nathan Sutton
nate commented on 3756a3f March 25, 2012

I don't know, guys, I really think the bike shed should be blue.

Richo Healey

Aside from anything else, this is the second epic PR disaster for rails in as many weeks (Mass assignment is a good thing, remember?)

The totalitarian thing is going to push people away.

Jamie Gaskins

@richoH You and I seem to have different definitions of "epic" and "disaster". This is a minor detail getting blown out of proportion.

Luke Chadwick

-1. Someone needs to bring all the other core team members into this conversation.

Jack Chen

Hey guys, I propose we change ActiveRecord::FinderMethods#exist? to return the ID of the row if it exists instead of true.
Why? I don't know, I felt like it.

Richo Healey

@jgaskins: I'm not sure how you percieve those terms, but the sheer number of geeks poking their heads in to see how this is going to pan out should be an indication of what's about to happen.

As you were though.

Jamie Gaskins

@ricoH This is no more epic nor disastrous than the PUT/PATCH and CoffeeScript issues, yet they still went through and people accepted them. This is simply another example of people disagreeing with something on the internet. The fact that it's been this way for a year and a half without a peep about it speaks to me far more than the volume of nerd rage in this or any other thread on the issue.

Luke Chadwick

@fxn, as requested I've created a pull request #5580 that is specifically to improve the performance of jruby and rubinius.

Jack Chen

I've created a pull request to change ActiveRecord::FinderMethods#exist? to return the id or nil.

#5582

Jason King

The PR, and all the discussion of it, and subsequent revert and all discussion here, are all the evidence anyone needs that Rails is an awesome framework. Clearly it makes actual work so easy that all you guys have too much time on your hands.

MenTaLguY

-1. I'd even be willing to take a performance hit to return a sensible value (i.e. a boolean) from a predicate method, though I don't see any convincing evidence that performance was an issue here in the first place.

Adrian Pacała

+1. After applying this patch my cloud-powered social web app became 20% more roflscalable.

Vesa Vänskä

-1. It would be nice if the return values would not make people confused. What does the 0 mean?

Steven Hancock

@txn Ok, maybe my last comment was a little out of line.. but yes, I have heard of docrails, the one patch I actually did get into Rails core back when I thought about contributing was through the docrails repo. I'll also admit (as I already kind of did) that the PR mentioned wasn't the best example of "reasonable documentation" either.. but you can't tell me it doesn't happen because I've seen it happen. Admittedly, this was back in the Lighthouse days, but I've seen perfectly good patches (code and docs) get flat-out rejected for no good reason.. or worse, code patches that sat there ignored until they would no longer cleanly apply and then rejected because they wouldn't apply. I know a lot has changed back then, and a lot of the problem did have to do with the difference in workflow.. but this arbitrary decision to revert a perfectly good patch that hurts nothing and helps performance just because you don't like the stated reason for the patch is also the kind of thing that pushes contributors away. If anyone has a right to be dictatorial or to say it's his framework and his code (as if it belongs to nobody else, and as you said pretty plainly in the other discussion), it''s DHH and nobody else, but even that's arguable when Github says there are 1299 contributors.

I guess I've always thought of opensource as a community effort.. and in a community we listen to each other and help each other, we don't arbitrarily break things for no good reason just because we can (which is exactly what introducing a performance regression, and then deliberately reverting the patch that fixes that performance regression, does). I do contribute to a few other projects, at least one is pretty big, and the core developers there love me (and it is a project based on Rails), they treat me and every other contributor and user of their code like an equal and value our opinions and our code, they don't arbitrarily decide to revert a good patch because they don't like the description.

Steven Hancock

By the way, I'm in no way saying that the entire Rails core team is dictatorial, or even that you are all the time (you usually don't display the attitude I've seen in these discussions over the past couple days, at least not that I've seen). You're all smart and talented individuals and generally friendly and helpful (and like I said, I think a lot of those instances that did turn me off on contributing in the first place probably had to do with the workflow at the time, and the actual amount of work that was going on at the time).. but there are times, like the past few days with this discussion, when one or two core team members do something that makes absolutely no sense to the vast majority of the Rails community "because they can" and it usually takes a cooler-headed core team member to fix it, if it gets fixed at all.

Ju Liu

I understand that it is perfectly fine Ruby code, but I really dislike the lawyerism "it does not return a singleton because we didn't use fixed width font in the doc".

Ryan Bigg

I am of the super-strong opinion that query methods should return only true or false. What if someone uses === true to check for the return value of this? It would fail.

Eloy Durán

@radar Then they would have a bug in their code, which has been the point all along.

Peter Wagenet

:-1: I had thought some progress was actually made on this topic. I guess I was mistaken.

Bradley Schaefer

Remember, -1 evaluates to true, so vote nil.

Seriously, though, there seems to be no harm in returning actual boolean values here and potential (although unlikely) harm in returning a truthy value. Why not make it airtight? I don't think it is "wrong" to use truthiness in ruby applications, but widely-used libraries should strive to eliminate potential misunderstandings. Yes, this is bikeshedding, but I think there is a philosophical debate underlying the issue that is worth discussing.

When is it okay to be truthy?

Atwam

I think @fxn forgot one usage of predicate methods : debugging.
Rails is so popular now that some people start using it even though they don't know ruby enough to be aware that 0 is truthy.
And guess what, some people may use the good old puts or render :text => request.xhr? to debug their controllers (I know, they should use tests everywhere, and ruby-debug is magic).

Well, I can only imagine what they'll think when they see 0 for request.xhr?.
Rails is too popular now to have this kind of prone-to-errors api (especially since this function is used a lot and isn't just an obscure protected predicate method that only people peeking-at-the-source would use).

Tony Arcieri

The real issue here, IMO, is least surprise:

>> request.xml_http_request?
 => true

:thumbsup:

>> request.xml_http_request?
 => 0

o_O ?

0 is not... least surprising. I'm hard pressed to think of more confusing results. :false or "false" perhaps.

Xavier Noria
Owner
fxn commented on 3756a3f March 26, 2012

@tarcieri you are too easily surprised :) (I hope you don't mind a little joke, no offense).

Tony Arcieri

@fxn thing is I'm not the only one...

Andrés Mejía

If it were in my hands, I would take back that Ruby Hero Award.

Steven Hancock

@alloy interestingly enough, in another commit where the idea of enabling attr_accessible by default in Rails (which fixes an actual security problem, not a perceived semantics problem) was being discussed, people were saying "that would beak existing apps" or "that would break existing documentation" and my argument was that if it breaks existing apps, that's a bug in their code. That wasn't a compelling enough argument to fix an acutal security issue (at least, not without bumping the version to 3.3), but it's a compelling enough argument to fix a perceived semantics issue when there was absolutely nothing wrong with the original code (As I said, double-negation is as much a Ruby idiom as using the =~ as a boolean operator is, so if this is correct then so was the original code).

mipearson

Is the following not a preferable implementation, or am I missing somethign amidst all the boolean literal discussion?

@env['HTTP_X_REQUESTED_WITH'].downcase.include?('xmlhttprequest')
Steven Hancock

@mipearson Personally I think returning any truthy value other than 0 is a preferable implementation when we're talking about the only ? predicate method that relates directly to the use of javascript and AJAX requests. Should people be relying on the return value of a ? predicate method being anything other than truthy of falsy? Absolutely not. Will any amount of documentation or education stop people from doing the wong thing? Again, absolutely not, look at how many people still don't use attr_accessible in their models and that's been a known issue for how long now? Sure, it's a different issue.. but it's still an issue of people doing the wrong thing because it's easier. Since Rails is supposed to optimize for "developer happiness" (is that still a Rails convention?) and we know that at least some developers will want to rely on the return value of this one predicate method because it's easier (and, therefore, makes them happier) this method should not return 0.

edit: typo

Eloy Durán

@stevenh512

but it's a compelling enough argument to fix a perceived semantics issue when there was absolutely nothing wrong with the original code (As I said, double-negation is as much a Ruby idiom as using the =~ as a boolean operator is, so if this is correct then so was the original code).

I agree that both Ruby idioms are just as correct for the implementation of a predicate and I have no problem against it returning true or false, but what I’m saying is that in general in Ruby any expectancy on a predicate to return the singletons is a bug in your code. Unless, of course, it is explicitly stated that the API is to return one of the singletons. If it isn’t specified and you do expect it to return either true or false than that’s a bug in your code, which is what I said to @radar.

The ‘result of a predicate method’-issue has to do with Ruby the language, not Rails the framework which your attr_accessible argument is about. I.e. When a pure Rails API such as attr_accessible is changed it might breaks apps, but at least you will know what to expect from it in the future, as it is only one framework specific method. Changing a predicate to return true or false, however, does not change what you can expect in the future of any other predicate method you might encounter in Ruby code (e.g. non-Rails code), which you simply can’t as that’s how Ruby libs have been built for years.

To be clear, I might have tripped up over the docs mentioning that it would return ‘true’ too, but if the project documents that it will specify the singleton true without regular font as being simply a ‘truthy’ value, then it’s simply my code at fault. I might still have brought that documentation issue up, but not regarding the code.

Btw, here’s a nice blog article someone wrote regarding these issues: http://blog.leshill.org/blog/2012/03/25/a-question-of-truth.html

Pavel Pravosud
rwz commented on 3756a3f March 27, 2012

I actually see zero reasons to revert this commit except for "I never said it should return boolean, so fuck you all and don't touch my code."

It won't hurt if predicate method returns boolean. It's actually a better practice since most people expect boolean from it.

-1 on this commit.

Stephen Celis

This negatively impacts performance. Regexp#=== is faster than String#=~:

>> [Benchmark.ms { 10_000.times { "test" =~ /t/ } }, Benchmark.ms { 10_000.times { /t/ === "test" } }]
=> [6.036, 4.037]
Peter Wagenet

@stephencelis The Rails Core Team has already pointed out that this is only a micro optimization which is not something they are particularly interested in. I still disagree with returning 0 from a predicate method, even though I do understand that it is technically correct. However, this ship seems to have sailed. I am disappointed with the result, but I don't think it's something worth fighting about anymore.

Andrés Mejía

@stephencelis, a lot of people have brought that point already, but @fxn doesn't care about it. It looks like he thinks it's a good thing to make the code slower and less clear just so he can win the argument and satisfy his whim.

Stephen Celis

I just wanted to provide an actual benchmark.

I agree with @fxn that that predicate methods do not need to return true or false in Ruby, but those that return a non-boolean value return a value that is useful. Returning 0 here is not useful, not even as a performance enhancement (because it is in fact is the opposite). Let's leave our egos at the door and change the code for the better.

Luke Chadwick

@andmej, @stephencelis I've reflected a lot on this issue over the last two days. I believe @fxn is doing the right thing. @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.

To his credit he has been responding patiently to people even when the discussions turned ugly. Suggesting that he is doing this without the best of intentions is wrong.

Stephen Celis

@vertis So we let it flop backwards this time? These are the arguments I'm hearing for the original:

  1. Ruby predicate methods do not need to return true or false.
  2. The original code that prompted the change to return true or false again was problematic code.

I agree with both of these, but they are both fallacious when used to argue for the Rails code in question.

  1. Argues: Ruby predicate methods do not need to return true or false. xhr? is a Ruby method. Therefore, xhr? does not need to return true or false.
  2. Argues: It is problematic to expect that a predicate method returns true or false. This expectation is a problem. Therefore, xhr? returning 0 is not a problem.

Here is some logic that hopefully supports making xhr? return true or false:

  1. I can't think of a single predicate method in the standard lib that returns something other than true, false, or a useful value. The fact that nonzero? returns self or nil can be useful. The fact that defined? returns nil or a string describing the object can be useful. However, 0 is not a useful value here. Can someone argue that 0 or nil is more useful than true or false? In lieu of a useful return value, we should return true or false.
  2. String=~ is slower than Regexp#===. It may be a minor optimization here, but elsewhere it could add up, so if you don't need the index of the match, why fetch it?

These are just problems that should be tidied up.

Scott Gonyea

Also, if statements are faster when evaluating true/false than they are with objects/nil. So, @fxn might as well be responsible for the burning of a square mile of rain forest. Monster.

Xavier Noria
Owner
fxn commented on 3756a3f March 27, 2012

@stephencelis regarding singletons: we do not care about the return value, whether the exact value is useful or not is irrelevant to us. We care about the quality of the implementation: delegating to a boolean operator trumps double negation for our taste (perhaps not yours, that'd be fine). Thus this commit is reverted because from our perspective there is nothing to fix regarding return values, the existing predicate is just fine.

Regarding performance, please see #5580.

I am disabling notifications, I have explained this many times, in many ways. Thanks to all.

Tony Arcieri

To put this in perspective, an app somewhere I work was broken by the original change in 0aa66f0. I don't think 0aa66f0 was warranted.

Perhaps the discussion should move to 0aa66f0.

mipearson

For what it's worth, I retract my comment re downcase.include? vs regexp: I had erroneously assumed that a regexp was significantly costlier (both in object allocation and speed), and that regexps in Ruby were discouraged unless necessary for readability reasons.

Luke Chadwick

@tarcieri is monkey patching for the specific app not an option?

Xavier Noria
Owner
fxn commented on 3756a3f March 28, 2012

@tarcieri That commit is one year and a half old. That commit has changed no contract, if an app "was broken" you need to fix the app. No monkey patching is needed if the source code is yours. You need to get a singleton if you rely on having one. This was already clear on Friday.

Can we carry on please?

Robb Shecter

And further: All of the arguments in favor of the latest change are based on Ruby. However, we're working on Rails here. We have Rails conventions. We can make something better.

Christoph Jasinski

@dogweather I strongly disagree. It's Ruby that made/make Rails great/strong not the other way round. Ruby is the source not the end product like Rails is.

Miles Georgi

The original commit should have been rejected, but didn't merit a revert, except for possible performance reasons.

Eugene Diachkin
ineu commented on 3756a3f April 18, 2012

Had really good time reading threads. This is the first time I see man defending his position so bravely despite community opinion and common sense. Respect to @fxn

Hrvoje Simic

-1 on this one. return true or false for methods that end up with ?

Michael Nikitochkin
miry commented on 3756a3f March 12, 2013

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

Please sign in to comment.
Something went wrong with that request. Please try again.