Conversation
For non Sting scalar values the object class gets injected in the signature data. Ruby 2.4 unified Fixnum and Bignum into Integer so a signature contening numbers in 2.1 will be different from one generated in 2.1. If you'll use Crimp to generate cache/database ids this can be a problem. This commit makes Crimp backward compatible.
Also to cope with TLSv1 issue.
lib/crimp/version.rb
Outdated
| @@ -1,3 +1,3 @@ | |||
| module Crimp | |||
| VERSION = '0.1.2'.freeze | |||
| VERSION = '1.0.0'.freeze | |||
There was a problem hiding this comment.
Should we keep this change at 0.x and then have the new version at 1?
There was a problem hiding this comment.
Pushed to 1.0.0 with the idea to set the new version as 2.0.0 but probably you're right, the current version is really not a 1.x thing!
|
Why do we require the version in the crimp.rb file when it doesn't use it?
|
| specify { expect(subject.signature(hash)).to be_a String } | ||
|
|
||
| it 'returns MD5 hash of stringified Hash' do | ||
| expect(subject.signature(hash)).to eq(Digest::MD5.hexdigest(subject.stringify(hash))) |
There was a problem hiding this comment.
This is not testing anything 😢
https://github.com/BBC-News/crimp/blob/dbc3db837c1e6d019fd779cc8e3447de2dedf532/lib/crimp.rb#L6
ettomatic
force-pushed
the
legacy-compatibility
branch
from
dffca70 to
cdbe0d9
Compare
junior451
left a comment
junior451
left a comment
There was a problem hiding this comment.
Looks good to me 👍
But its there a reason why are all the methods in the Crimp module class methods?
|
@junior451 The public |
lib/crimp.rb
Outdated
| # to debug especially for nested data structures. | ||
| # | ||
| def self.integer_to_string(obj) | ||
| "#{obj}Fixnum" |
There was a problem hiding this comment.
Probably an edge case, but would previous invocations always be dealing with Fixnum's (as opposed to Bignum's as well)?
There was a problem hiding this comment.
yes you're right... I'll add another hack to handle this 😞 .
| let(:hash_with_numbers) { { a: { b: 1, c: 3.14 }, d: 'd' } } | ||
| let(:hash_unordered) { { d: 'd', a: { c: 'c', b: 'b' } } } | ||
| let(:array) { [1, 2, 3, [4, [5, 6]]] } | ||
| let(:array_unordered) { [3, 2, 1, [[5, 6], 4]] } |
There was a problem hiding this comment.
Does this cover the full range of data types you're wanting to test against? No booleans, nil values, non-UTF-8 character strings, etc?
There was a problem hiding this comment.
yes test coverage is weak, the new version of Crimp i'm working on has better tests tho.
coderkind
left a comment
coderkind
left a comment
There was a problem hiding this comment.
👍 looks good; I've added a few questions purely out of ignorance of how it's being/will be used, and having given the Node version some attention.
ettomatic
force-pushed
the
legacy-compatibility
branch
from
10ec655 to
46f1bb4
Compare
ettomatic
force-pushed
the
legacy-compatibility
branch
from
46f1bb4 to
43bfa4f
Compare
7 participants
Fix issue with different signatures between Ruby 2.1 and 2.4.
For non-String scalar values, the object class name gets injected in the
signature data.
Ruby 2.4 unified Fixnum and Bignum into Integer so a signature
containing numbers in 2.1 will be different from one generated in 2.4.
Ruby 2.1
Ruby 2.4.2
Floats are not showing the same issues, as both 2.1 and 2.4 are returning:
If you'll use Crimp to generate cache/database ids this can be a
problem.
This commit makes Crimp backwards compatible.
See also PR #8 for more context (thanks @coderkind and @Tarqwyn)