Ensure ids of resulting records are integers.

[frodsan]

Before:

user = User.create
user.id # => 1

user = User.all.first
user.id # => "1"

After

user = User.create
user.id # => 1

user = User.all.first
user.id # => 1

[djanowski]

There is another test to ensure that newly created models have an integer ID, so I think this could be rewritten to something like:

test "..." do |post, *comments|
  assert comments.map(&:id) == post.comments.map(&:id)
end

[frodsan]

I would change it, but I would like to finish our discussion about this. Ping me if you're in IRC 😄

[frodsan]

@djanowski done, please review again 😄

[frodsan]

Related #130

[djanowski]

Thanks @frodsan. I'm +1 on making ID an integer everywhere.

Waiting to hear from @cyx and @soveran.

[cyx]

Yep +1
On Jan 7, 2014 5:31 AM, "Damian Janowski" notifications@github.com wrote:

Thanks @frodsan https://github.com/frodsan. I'm +1 on making ID an
integer everywhere.

Waiting to hear from @cyx https://github.com/cyx and @soveranhttps://github.com/soveran
.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/137#issuecomment-31736966
.

[soveran]

I'm not 100% sure about converting ids to integers. I know it is convenient, but what makes me wonder if it's the right choice is the fact that we eventually convert every id to a string when we send it to Redis, so we pay the (very small) performance penalty twice. I checked how much slower it is to map to integers and it's more than ten times slower than just returning strings. That said, the time it takes to convert an array of a hundred strings is tiny.

On the other hand, I like the idea of making ids public.

[frodsan]

Maybe I can go back to my first implementation.

        data.each_with_index do |atts, idx|
+         result << model.new(Utils.dict(atts).update(:id => ids[idx].to_i))
-         result << model.new(Utils.dict(atts).update(:id => ids[idx]))
        end

At least, this ensures that the records have integer ids. The #ids methods still return an array of strings so I guess there's no performance penalty. wdyt?

[djanowski]

I'm +1 on making id expectable. I don't think having id always be an integer is super valuable. Rather, knowing that it's always a string or always an integer is valuable.

Maybe it's better to always ensure it's a string? (In the past, when we used redis-rb, this would only require us to to_s the ID when we called INCR on create. But now that Redic doesn't do castings, the result of INCR should be a string anyway, right?).

What do you think?

[frodsan]

👍 to making id expectable.

[soveran]

@djanowski Right now we would have to change one line in save.lua, as it returns the id as an integer. Tiny change.

[frodsan]

@soveran so, 👍? :p

[frodsan]

Closing in favor of #141