Make ID <=> String equality symmetrical #821
Conversation
no-reply
force-pushed
the
string-equality
branch
3 times, most recently
from
1579ce6 to
aaa6da5
Compare
no-reply
force-pushed
the
string-equality
branch
from
aaa6da5 to
343656c
Compare
|
do we need to make definition of |
Since we now support symmetric equality with `String` objects of the same lexical value, there's no need for a specialized ID class specific implementation. Instead, we always cast `self` with `#to_str` and delegate `#==` to `other`. In the case we are comapring two `Valkyrie::ID` objects, `id_1 == id_2`, we cast the first and compare using `id_2 == id_1.to_str`; in turn, this casts `id_2` and we end up with `id_1.to_str == id_2.to_str`. This sounds complicated but it involves fewer comparisons and method calls that the old implementation, which compared both `id_1.class == id_2.class` and then ran `Array` equality via `id_1.state == id_2.state`.
| end | ||
|
|
||
| it 'is equal to the equivalent string' do | ||
| expect('123' == resource.thumbnail_id).to be true |
There was a problem hiding this comment.
I think for completeness, I would also add:
expect(resource.thumbnail_id == '123').to be trueThere was a problem hiding this comment.
that's the test on https://github.com/samvera/valkyrie/pull/821/files/d9f0895504c17dcd3056e783a24d7bab12742642#diff-b8273acc787c804ca17fe99782a00941R61, right?
this version needs to embed the equality check in the expectation to satisfy rubocop-rspec, which gets mad if you put a literal value in expect's actual. expect('123').to eq resource.thumbnail_id is equivalent, but fails style check (:
|
I have to run this through Figgy and see if it works okay without being in the conditional config. I don't really have the bandwidth atm, but I'll get to it when I can. Thank you! |
|
would it be helpful if someone else could run the figgy test suite with this? |
|
Wondering how i can help move this forward? |
|
Sorry it has taken so long. I'm fiddling with it now, but I've actually discovered Figgy's throwing a ton of the deprecations, which makes me think there's some underlying library issue doing direct compares. I'm poking around. |
|
https://github.com/samvera/valkyrie/blob/master/lib/valkyrie/types.rb#L87 is throwing a deprecation error in our test suite, @no-reply can you just switch the order of those checks as part of this PR? |
|
@no-reply I can confirm this works in Figgy. If you make that switch in the check above I'd be happy to merge. |
|
I applied that change and it looks like that deprecation message never gets hit anymore. I'll have to look at adding a test or something, but have to move to local stuff for today. |
Fixes #820