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

Fixes #11454 . We should define the return type of select_all method clearly. #11461

Merged
merged 2 commits into from Jul 22, 2013

Conversation

kennyj
Copy link
Contributor

@kennyj kennyj commented Jul 16, 2013

Fixes #11454.

Now select_all and select methods return an ActiveRecord::Result instance.
I think we should define the return type of select_all method clearly, and fix related comments.

@@ -18,8 +18,7 @@ def to_sql(arel, binds = [])
end
end

# Returns an array of record hashes with the column names as keys and
# column values as values.
# Returns an ActiveRecord::Result instance with columns and rows methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's enough to say: "Returns an ActiveRecord::Result instance". However we should improve the documentation of ActiveRecord::Result.

@senny
Copy link
Member

senny commented Jul 19, 2013

Can you squash the commits and maybe also improve the docs on ActiveRecord::Result? That would be awesome 💛

@kennyj
Copy link
Contributor Author

kennyj commented Jul 22, 2013

hi @senny.
I've squashed the commits.

I also try to add some usage comments. Please some advise for me :)

# result = ActiveRecord::Base.connection.exec_query('SELECT id, title, body FROM posts')
# result # => #<ActiveRecord::Result:0xdeadbeef>
#
# # Gets column name array.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reword this to: "Get the column names of the result:"

@senny
Copy link
Member

senny commented Jul 22, 2013

@kennyj looks great! 💛

@kennyj
Copy link
Contributor Author

kennyj commented Jul 22, 2013

thank you for your advises, @senny
I've updated this PR.

@senny
Copy link
Member

senny commented Jul 22, 2013

This looks good. Can you add [ci skip] to the documentation commit?

@kennyj
Copy link
Contributor Author

kennyj commented Jul 22, 2013

okay. I've already updated.

senny added a commit that referenced this pull request Jul 22, 2013
Fixes #11454 . We should define the return type of select_all method clearly.
@senny senny merged commit 2d50bc2 into rails:master Jul 22, 2013
@kennyj kennyj deleted the fix_11454 branch July 22, 2013 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AR 4.0.0 select_all() returns wrong object
2 participants