In IListOps.Fetch, instead of doing the protocol conversion manually,
you should calls Protocols. ConvertToInteger. Tomas, can you
double-check this?
In IListOps.First, you can get rid of the allocateStorage argument as
its not being used anymore. (You can batch this fix with your next
commit)
The rest looks good! Great to see array getting more compliant!
Btw, could you please increase the diff context size to 15-20 so more of
the surrounding lines are included? That makes it easier to understand
what the fix is doing when using the url below, without having to map
that back to the correct line number in the actual file.
Yes, Shri is right in essence - you shouldn’t do protocol conversions
manually. The conversion used by DefaultProtocol on int is
Protocols.CastToInteger however (ConvertToInteger performs different
integer conversion).
In IListOps.Fetch, instead of doing the protocol conversion manually,
you should calls Protocols. ConvertToInteger. Tomas, can you
double-check this?
In IListOps.First, you can get rid of the allocateStorage argument as
its not being used anymore. (You can batch this fix with your next
commit)
The rest looks good! Great to see array getting more compliant!
Btw, could you please increase the diff context size to 15-20 so more of
the surrounding lines are included? That makes it easier to understand
what the fix is doing when using the url below, without having to map
that back to the correct line number in the actual file.
In IListOps.Fetch, instead of doing the protocol conversion manually, you
should calls Protocols. ConvertToInteger. Tomas, can you double-check this?
Hmm, right, and the code looks much more cleaner too.
I think I should just use Protocols.CastToFixnum then: we don’t really
care about bignums here, they can’t be used as indexes for arrays
anyway and in fact MRI throws a RangeError “bignum too big to convert
into `long’”.
In IListOps.First, you can get rid of the allocateStorage argument as its
not being used anymore. (You can batch this fix with your next commit)
Same for IListOps.Last, it just slipped away. I removed allocateStorage
on both.
Btw, could you please increase the diff context size to 15-20 so more of the
surrounding lines are included? That makes it easier to understand what the
fix is doing when using the url below, without having to map that back to
the correct line number in the actual file.
I guess there is no way to increase the lines range of a diff viewed
via the github web interface, but I can generate and attach custom
diff patches to my mails if you think it could be useful, just like
the one attached to this mail and generated with "git diff HEAD~1 -U15
patch.diff" (PS: I have not yet pushed this commit on my remote
repository).
Result of “git diff HEAD~1 -U15” would be nice if you can remember to
include it, but don’t worry about it too much. I will try to deal with
the github.com web view and see if it bothers me too much.
Yes, Shri is right in essence - you shouldn’t do protocol conversions
manually. The conversion used by DefaultProtocol on int is
Protocols.CastToInteger however (ConvertToInteger performs different
integer conversion).
In IListOps.Fetch, instead of doing the protocol conversion manually,
you should calls Protocols. ConvertToInteger. Tomas, can you
double-check this?
In IListOps.First, you can get rid of the allocateStorage argument as
its not being used anymore. (You can batch this fix with your next
commit)
The rest looks good! Great to see array getting more compliant!
Btw, could you please increase the diff context size to 15-20 so more of
the surrounding lines are included? That makes it easier to understand
what the fix is doing when using the url below, without having to map
that back to the correct line number in the actual file.
In IListOps.Fetch, instead of doing the protocol conversion manually,
you should calls Protocols. ConvertToInteger. Tomas, can you
double-check this?
In IListOps.First, you can get rid of the allocateStorage argument as
its not being used anymore. (You can batch this fix with your next
commit)
The rest looks good! Great to see array getting more compliant!
Btw, could you please increase the diff context size to 15-20 so more of
the surrounding lines are included? That makes it easier to understand
what the fix is doing when using the url below, without having to map
that back to the correct line number in the actual file.
Shri
This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.