mime-types / ruby-mime-types

Ruby MIME type registry library
Other
322 stars 122 forks source link

Define MIME::Type#hash #167

Closed ajvondrak closed 2 years ago

ajvondrak commented 2 years ago

Fixes #166. See that issue for the relevant context.

Upon studying the definition of MIME::Type#eql?, I determined that we shouldn't include self.class in the hash. #eql? uses an #is_a? check, which will succeed even on subclasses of MIME::Type. So #eql? doesn't require self and other be of the exact same class. If we were to include self.class in the hash, then it would erroneously make the hashes different for two #eql? objects of different classes (since different classes generally hash to different values). I added an extra test around MIME::Type#eql? to codify this behavior. While I was in there, I also fixed a typo in an existing MIME::Type#eql? test. πŸ˜…

Note that there aren't many generic tests we can make around #hash itself. Even two distinct MIME::Type instances could still hash to the same value, in principle. So there are only really tests around instances that should be #eql? to each other.

One clue we have that this PR addresses the randomness of warnings seen in #163 is when I run rake with/without these changes locally. :)

On main, I see no warnings about the duplicate application/netcdf data, because the Set treats the two instances as distinct (unless we get lucky, which has also happened to me intermittently):

Without hash definition ```console $ git branch define-mime-type-hash * main $ rake Run options: --seed 63590 # Running: ................................................................................................................................................... Finished in 2.143452s, 68.5810 runs/s, 137.1619 assertions/s. 147 runs, 294 assertions, 0 failures, 0 errors, 0 skips rm -rf doc rm -r pkg ```

On the topic branch, I see all the warnings ‼️ now that the two instances hash the same:

With hash definition ```console $ git branch * define-mime-type-hash main $ rake Type application/netcdf is already registered as a variant of application/netcdf. Run options: --seed 24550 # Running: .......Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. ............................Type application/netcdf is already registered as a variant of application/netcdf. .................Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. ................Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. ........Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .Type application/netcdf is already registered as a variant of application/netcdf. .........Type application/netcdf is already registered as a variant of application/netcdf. Type application/netcdf is already registered as a variant of application/netcdf. ............................................... Finished in 3.228527s, 46.7706 runs/s, 92.3022 assertions/s. 151 runs, 298 assertions, 0 failures, 0 errors, 0 skips rm -rf doc rm -r pkg ```

This change is Reviewable

halostatue commented 2 years ago

Thanks for the contribution! I’m going to delay releasing this a bit, as I would like to figure out how to resolve the load issue.

ajvondrak commented 2 years ago

Makes sense. πŸ‘ Glad I could help!