#import method conflicts

[itay-grudev]

elasticsearch-rails uses the method import for importing data into ES. And while I am contradicted about which library holds stronger right to the import keyword, would you consider renaming the method to bulk_import or at least create an alias? I believe in general it would be nice to not use such a generic method name for an external library with such a specific function.

[jkowens]

@zdennis this seems to be reported enough that I'm wondering if we should consider renaming import to something like bulk_import as suggested and then creating an import alias. What do you think?

[jkowens]

Here's a quick stab at an update that should maintain backwards compatibility: master...jkowens:alias_import_method

cc: @zdennis

[itay-grudev]

That's great. The alias if responds_to? is a quite clever hack. And I'm REALLY impressed by your quick response.

[jkowens]

Haha thanks, but I have to credit the Ransack team for that nifty trick. They do something similar with the search method added to ActiveRecord::Base.

[zdennis]

@jkowens, this is great. My only question is should we Use the second argument to ‘respond_to?’ to be extra safe that we aren’t clobbering anything?

[itay-grudev]

@zdennis This should be alright, as the second argument is for private methods. And even if there is an import method in a private scope, it doesn't concern us. We only care if there is an import method in the public scope in which case we try not to override it.

[jkowens]

@zdennis that seems like a good thought to me. We might as well try to avoid clobbering private methods if possible. I’ll go ahead and update the PR.

[jkowens]

@zdennis interestingly, making this change reveals that JRuby provides a private import method (our tests failed).

See https://github.com/jruby/jruby/blob/master/core/src/main/ruby/jruby/java/core_ext/module.rb#L54

I'm a bit on the fence now. If we don't create an import alias for apps with ActiveRecord objects that have private import methods, that could lead to surprise failures in existing apps. Although clobbering that method probably isn't the best thing either.

[itay-grudev]

I believe the best solution is to keep it as is, update the documentation to explicitly recommend using bulk_import and update the tests to use bulk_import so they pass.

Only a very small amount of JRuby people will notice a problem if they are importing modules in their ActiveRecord models.

And you can drop the old import method alias the next time you are doing a major release.

[jkowens]

@zdennis I think I'm going to with what I believe @itay-grudev is suggesting and only check the public method scope when adding the import alias. That will most closely match current functionality and not introduce any surprises. We can either add the check for private scope or completely remove the import alias in a future major release. One day we'll have to go all in and release a 1.0 😂