-
-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Define MIME::Type#hash #167
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
halostatue
approved these changes
Jul 4, 2022
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. |
halostatue
added a commit
that referenced
this pull request
Jul 4, 2022
Makes sense. 👍 Glad I could help! |
halostatue
added a commit
that referenced
this pull request
Aug 7, 2023
- 1 bug fix: - Added a definition of `MIME::Type#hash`. Contributed by Alex Vondrak in [#167][], fixing [#166][]. - Dependency and CI updates: - Update the .github/workflows/ci.yml workflow to test Ruby 3.2 and more reliably test certain combinations rather than depending on exclusions. - Change `.standard.yml` configuration to format for Ruby 2.3 as certain files are not properly detected with Ruby 2.0. - Change from `hoe-git` to `hoe-git2` to support Hoe version 4. - Apply `standardrb --fix`. - The above changes have resulted in the soft deprecation of Ruby versions below 2.6. Any errors reported for Ruby versions 2.0, 2.1, 2.2, 2.3, 2.4, and 2.5 will be resolved, but maintaining CI for these versions is unsustainable. [#166]: #166 [#167]: #167
Merged
halostatue
added a commit
that referenced
this pull request
Aug 7, 2023
- 1 bug fix: - Added a definition of `MIME::Type#hash`. Contributed by Alex Vondrak in [#167][], fixing [#166][]. - Dependency and CI updates: - Update the .github/workflows/ci.yml workflow to test Ruby 3.2 and more reliably test certain combinations rather than depending on exclusions. - Change `.standard.yml` configuration to format for Ruby 2.3 as certain files are not properly detected with Ruby 2.0. - Change from `hoe-git` to `hoe-git2` to support Hoe version 4. - Apply `standardrb --fix`. - The above changes have resulted in the soft deprecation of Ruby versions below 2.6. Any errors reported for Ruby versions 2.0, 2.1, 2.2, 2.3, 2.4, and 2.5 will be resolved, but maintaining CI for these versions is unsustainable. [#166]: #166 [#167]: #167
halostatue
added a commit
that referenced
this pull request
Aug 7, 2023
- 1 bug fix: - Added a definition of `MIME::Type#hash`. Contributed by Alex Vondrak in [#167][], fixing [#166][]. - Dependency and CI updates: - Update the .github/workflows/ci.yml workflow to test Ruby 3.2 and more reliably test certain combinations rather than depending on exclusions. - Change `.standard.yml` configuration to format for Ruby 2.3 as certain files are not properly detected with Ruby 2.0. - Change from `hoe-git` to `hoe-git2` to support Hoe version 4. - Apply `standardrb --fix`. - The above changes have resulted in the soft deprecation of Ruby versions below 2.6. Any errors reported for Ruby versions 2.0, 2.1, 2.2, 2.3, 2.4, and 2.5 will be resolved, but maintaining CI for these versions is unsustainable. [#166]: #166 [#167]: #167
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #166. See that issue for the relevant context.
Upon studying the definition of
MIME::Type#eql?
, I determined that we shouldn't includeself.class
in the hash.#eql?
uses an#is_a?
check, which will succeed even on subclasses ofMIME::Type
. So#eql?
doesn't requireself
andother
be of the exact same class. If we were to includeself.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 aroundMIME::Type#eql?
to codify this behavior. While I was in there, I also fixed a typo in an existingMIME::Type#eql?
test. 😅Note that there aren't many generic tests we can make around
#hash
itself. Even two distinctMIME::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 theSet
treats the two instances as distinct (unless we get lucky, which has also happened to me intermittently):Without hash definition
On the topic branch, I see all the warnings‼️ now that the two instances hash the same:
With hash definition
This change is