Conflicts with hash_diff

[jfelchner]

Note: If you've upgraded hashdiff, made sure that you've upgraded your references, and want to silence the warnings, you can manually opt-in to the 1.0 beta by following the instructions here

The name of this gem is hashdiff. There is another gem named hash_diff. When they are both required via dependencies, errors are thrown. Additionally since the behavior is different, gems that are relying on hashdiff's behavior, may get hash_diff's behavior and vice versa.

The reason for the conflict is because, based on ruby conventions, _'s are the separators for camel case.

So:

'hashdiff'.camelize # => "Hashdiff"

'hash_diff'.camelize # => "HashDiff"

Unfortunately instead of making the base module of this gem Hashdiff, it's called HashDiff and it conflicts.

[liufengyun]

This is indeed an oversight. It is too late to change the name now, I guess.

[jfelchner]

You don't need to change the name, you need to change the constants.

Rename them all from HashDiff to Hashdiff, then you rename lib/hash_diff.rb to lib/hashdiff.rb.

You create a new hash_diff.rb and in it, put:

require 'hashdiff'

HashDiff = Hashdiff

Done. Backwards compatible.

[liufengyun]

Thanks for the tip @jfelchner . I'm not familiar with Ruby naming & file loading mechanism, could you please elaborate a little more on how does Ruby decide which file to load: lib/hash_diff.rb or lib/hashdiff.rb?

[jfelchner]

When you do require ‘hash_diff’ vs require ‘hashdiff’

But now that I’m thinking about it it would have to be named differently or there would still be a conflict.

Lemme do a PR for you.

[luke-hill]

Just to add my 0.02c here.

This gem has 30,000,000 downloads. The other gem has 50,000 and hasn't been updated in a year.

I would say sensibility in some sense has to outweigh changing for changings sake.

[jwilger]

Just to add my 0.02c here.

This gem has 30,000,000 downloads. The other gem has 50,000 and hasn't been updated in a year.

I would say sensibility in some sense has to outweigh changing for changings sake.

Except that I just had to go figure out WTF my diffs were coming out wonky in a project that ended up including both gems, one as a dependency of another internal gem, and one as a dependency of webmock. Had I not been the developer that introduced one of the dependencies, I would have had no effing clue where to start and probably chucked my computer out a window and contemplated my life choices.

(By the way, it happened because, even though I wanted this library in the other internal gem, I accidentally put hash_diff in its gemspec, because that's the convention if the library's constant is HashDiff.)

+1 for changing the constant name in this library to match the convention.

[liufengyun]

I feel sorry to hear that @jwilger . I'm open to a change that can avoid the conflict. Maybe with a major version change, so that users are warned that the version has breaking changes.

[jfelchner]

@liufengyun I would really like to get a major version bump out there so that we can fix this. I'd be happy to do the PR.

[liufengyun]

@jfelchner A PR is welcome, I'm definitely for it.

[pboling]

Changing the namespace of a gem in a major bump is super normal. Witness Rspec => RSpec, and FactoryGirl => FactoryBot. It's not a big deal, and can actually be a good thing. People should be careful with major version drops, and a namespace change forces the issue.

[jfelchner]

@pboling I agree, but this is a special case because there is a gem we don't control using that constant. In the case of FactoryGirl and Rspec, they controlled those constants. Doing this: #45 (comment)

Wouldn't solve the problem. Because existing gems are accessing the hashdiff.rb file, which they would continue to require in the new version (because no one has told them otherwise). If the next version's hashdiff.rb doesn't define HashDiff, then they will start to break (requiring a major version bump). If the new hashdiff.rb does define HashDiff = Hashdiff, then we end up with the exact same issue we have right now (redefining a constant).

It also wouldn't solve the problem if everyone just used the shim (which most people would do) by requiring hash_diff.rb instead of hashdiff.rb. Because the warnings would still exist.

The best path forward here is to update the constants, add a shim, and then add a warning to the output that the next version will change the constant.

Then in a few weeks, release a major version bump where the only difference is that the shim is removed.

[pboling]

Because existing gems are accessing the hashdiff.rb file, which they would continue to require in the new version (because no one has told them otherwise).

I expect this to be rare. Most users of this gem will load it via bundler. Most people won't use hard coded requires, or a shim, and will just take whatever the default is from bundler.
Update: I was wrong.

Aside from that I totally agree on the best path forward.

[jfelchner]

@pboling Bundler will automatically require the file with the same name as the gem: hashdiff.rb which doesn't change anything in this case.

Regardless of if Bundler loads it or not, if hashdiff.rb doesn't define HashDiff, existing gems will break unless they update their constant. If it does define HashDiff, then we'll still get the "redefining constant" error, which leaves us where we are.

[pboling]

I see. I also realized that many other gems probably depend on HashDiff (I'm in the process of writing one) and we do use explicit requires in some places, like spec_helper.rb... 👍

[jfelchner]

@liufengyun PR is complete. The require paths were correct, thus the only change that should need to be done by people using the library is to do a find and replace for HashDiff with Hashdiff.

[jfelchner]

@liufengyun go ahead and cut a 0.4.0 release with my PR changes in it. Once that's done, we'll need to wait about two months and then we can release the 1.0 release.

[liufengyun]

I just cut 0.4.0, thanks for proposing the fix @jfelchner 🎉

[krainboltgreene]

I don't mean to butt in, but this just broke vcr test specs: https://travis-ci.org/vcr/vcr/jobs/538555321

[krainboltgreene]

Is this something I can help hashdiff with or is this something I simply need to fix on VCR tests?

Note: We don't depend on hashdiff directly.

[jfelchner]

@krainboltgreene Yes, see the webmock issue above, which is probably where this is failing. They aren't pessimistically locking hashdiff and therefore it's causing this to be a bigger deal than it should be.

[krainboltgreene]

Understood, thanks for the prompt reply.

[jfelchner]

@krainboltgreene no worries. And sorry for the trouble. If you can see a better way to handle this, please let me know but I think this is the only way to do this.

If you all require hashdiff manually earlier in the test process before any tests are run, I believe that should eliminate this message. This message is only shown on the first require 'hashdiff' load.

[krainboltgreene]

I think this was a reasonable approach. I'm going to do as you advise and require hashdiff specifically.

…

On Tue, May 28, 2019 at 9:44 PM Jeff Felchner ***@***.***> wrote: @krainboltgreene <https://github.com/krainboltgreene> no worries. And sorry for the trouble. If you can see a better way to handle this, please let me know but I think this is the only way to do this. If you all require hashdiff manually earlier in the test process before any tests are run, I believe that should eliminate this message. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45?email_source=notifications&email_token=AACRXWMFVFWJYVF6VJDML3DPXYC2NA5CNFSM4GKRKEOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWOEU4A#issuecomment-496781936>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACRXWOO7KMNMZGHTZAF74LPXYC2NANCNFSM4GKRKEOA> .

-- Kurtis Rainbolt-Greene, Software Developer Founder of Difference Engineers 2985 San Marino St. Los Angeles, CA 90006 202-643-2263

[jfelchner]

@krainboltgreene I just want to verify that this is just a VCR test failure and the warning message isn't going to affect other VCR users who are mocking with webmock right? It looked like from that test failure that the warning was ending up inside the cassette data and that would be extremely bad if everyone's VCR cassettes started breaking if they're using webmock.

[luke-hill]

The failure is because of the warning messages we now fire. The actual code tests still passed; it's just one of the message expectations now has "Warning from 1.0 we will be moving"

[nacengineer]

I know it's a bit nitpicky and while I get that you want to get the message out about this change, passing it via warn seems excessive as it's not really a deprecation per se. By using warn it gets raised in every spec run. IMO it'd be better raised via a post install message.

[jfelchner]

Are you saying I need to force my project to load a beta version of the gem which I am not using?

You are using it if you're using a gem that's using it. If you weren't using it, webmock would break.

So if you want webmock to use the beta instead of using the released version (which is how you get rid of the deprecation), then you need to tell webmock to use the beta. That's what the solution above does.

If you are there is something very wrong with that workaround.

It's a "workaround" not a "final state", therefore it's a compromise between people who want the deprecation to go away and people who don't want their apps to break. We are giving people time to migrate. As you can see, we just got a notification an hour ago of someone who just now noticed the deprecation. Be empathetic to other developers who may not have found it yet.

And I just want to point out that it took you significantly more time (both yours and mine) to write your comment than it would have taken to add the line to your Gemfile, bundle update, resolve your issue, and move on. 😏

Is it technically hard to suppress the warning when there is no HashDiff referenced anywhere?

Although technically possible, it's significantly more error-prone than our current solution.

[nacengineer]

@gsar @jfelchner it's important to remember that the issue revolves are how ruby warn levels are set. This is analogous to something we all are accustomed to doing with the Logger class, we're just not used to doing it on the ruby level.

@gsar the burden to fix is on webmock. The burden to bear the warning is on the rest of us. You can change your ruby warn level in your Gemfile via $VERBOSE = nil or by setting it on the ruby itself. https://mislav.net/2011/06/ruby-verbose-mode/

[gsar]

@jfelchner if it was my personal pet project, i would have have implemented your workaround and moved on. the reality is that i have to do this for a codebase that has many other devs involved, and we have policies/processes around bringing in new code. we generally don't use "beta" code anywhere so i'd have to make an exception here and explain why it merits an exception, etc. that's what i meant by "more trouble than it is worth".

@nacengineer i'm not clear on what you mean by "burden to fix is on webmock". they have already fixed it by not using HashDiff. perhaps they need to be convinced to require the beta version in their released version so all the multitudes that use it don't have to do the same? turning off verbose mode is not really an option for me as that would squelch other warnings that i do want to see.

i just want the maintainers here to realize that "the ask" for the workaround here is by no means as simple as it sounds like for some of us to implement.

[jfelchner]

@gsar what is the burden that you're trying to resolve? Simply not seeing the warning? Is it causing your tests to fail?

webmock will update once we're fairly sure everyone has upgraded. I'll submit the PR myself.

[gsar]

@jfelchner i'm simply trying to avoid the work of looking into it and explaining it to their teams for other people in the same situation. it is not causing any test failures on my end, just some rubber-necking and the associated time wastage.

thanks for engaging with my concerns, much appreciated.

[schm]

Although technically possible, it's significantly more error-prone than our current solution.

This piqued my curiosity and I came up with the following patch. It's based on v0.4.0 and I'm not sure if it's worth a v0.4.1. But I wanted to leave it here anyway.

diff --git a/lib/hashdiff.rb b/lib/hashdiff.rb
index 1ba94b8..6012d2d 100644
--- a/lib/hashdiff.rb
+++ b/lib/hashdiff.rb
@@ -9,6 +9,4 @@ require 'hashdiff/diff'
 require 'hashdiff/patch'
 require 'hashdiff/version'

-HashDiff = Hashdiff
-
-warn 'The HashDiff constant used by this gem conflicts with another gem of a similar name.  As of version 1.0 the HashDiff constant will be completely removed and replaced by Hashdiff.  For more information see https://github.com/liufengyun/hashdiff/issues/45.'
+autoload(:HashDiff, 'hashdiff_deprecated')
diff --git a/lib/hashdiff_deprecated.rb b/lib/hashdiff_deprecated.rb
new file mode 100644
index 0000000..4290182
--- /dev/null
+++ b/lib/hashdiff_deprecated.rb
@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+HashDiff = Hashdiff
+
+warn 'The HashDiff constant used by this gem conflicts with another gem of a similar name.  As of version 1.0 the HashDiff constant will be completely removed and replaced by Hashdiff.  For more information see https://github.com/liufengyun/hashdiff/issues/45.'

[caduguedess]

I didn't got it, this Hashdiff warning is something I need to worry or not?
I'm trying to setup the Openstreetmap Website https://github.com/openstreetmap/openstreetmap-website
But when I run bundle exec rake db:create it gives me that Hashdiff warning with some errors, are the following errors related to it?
Here is a Gist with the command and error pasted.
https://gist.github.com/caduguedess/4cd6d9205f741e105722d53451bb46c2

[jfelchner]

@caduguedess not even remotely related.

[jfelchner]

@schm new approach! Interesting! I'm not 100% confident with my knowledge of Ruby's autoload, but we'll be releasing 1.0 here in the next week, and this should become a moot point.

[jfelchner]

@liufengyun #74 :)

[liufengyun]

It's merged and released, thanks a lot for your help to finally resolve the issue @jfelchner 👍