Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed request.xhr? to return boolean instead of 0 or nil #9670

Closed
wants to merge 1 commit into from

Conversation

miry
Copy link

@miry miry commented Mar 11, 2013

Changed return values for request.xhr? to true or false instead of nil and 0.

@luke-gru
Copy link
Contributor

not a bug, this PR has been made before (#5329) and has been rejected

@pixeltrix
Copy link
Contributor

@miry we've been here before - I suggest that you read the discussion on 3756a3f for the back story.

@pixeltrix pixeltrix closed this Mar 11, 2013
@miry
Copy link
Author

miry commented Mar 11, 2013

It is really confuse that a method with suffix ? does not return true or false. IMHO

Btw: https://github.com/rack/rack/blob/master/lib/rack/request.rb#L308

@pixeltrix
Copy link
Contributor

@miry whether a predicate should return true or false was debated to death in #5329 and 3756a3f - going over it again serves no purpose.

@miry
Copy link
Author

miry commented Mar 11, 2013

@pixeltrix What do you think to replace word in the comment from the true(or even all phrase) to something more close to solution?

http://apidock.com/rails/ActionDispatch/Request/xml_http_request%3F

@fxn
Copy link
Member

fxn commented Mar 11, 2013

That's how predicates are documented: "returns true if...", "says whether...". In the very exceptional cases when you really want to commit or refer to the true singleton, the documentation guidelines of Rails say you need to use fixed-width font. True and false in regular font are used all over the place.

@miry
Copy link
Author

miry commented Mar 13, 2013

@fxn What were the conditions to use the Regexp?

If only for case issue, so to use downcase is faster.

https://gist.github.com/miry/5150174

@fxn
Copy link
Member

fxn commented Mar 13, 2013

The method checks for case-insensitive inclusion, not sure if that is needed though. I didn't write the original method and don't see any test checking inclusion.

Bear in mind that no matter how you rewrite it, the method does not document singletons. So even in the case of a refactor that returned singletons you still could not rely on them, the same way people cannot rely on nil/0 today.

@miry
Copy link
Author

miry commented Mar 13, 2013

@fxn I don't care about return value now, just performance issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants