Skip to content
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

Issue deprecation warnings #45

Open
afred opened this issue May 16, 2018 · 11 comments
Open

Issue deprecation warnings #45

afred opened this issue May 16, 2018 · 11 comments

Comments

@afred
Copy link

afred commented May 16, 2018

Background: Solrizer has been absorbed by ActiveFedora. You should use ActiveFedora.index_field_mapper where you used to use Solrizer.

Done when: deprecation warnings are issued when attempting to use Solrizer.

@tpendragon tpendragon added the maintenance CCMWG prerequisites label Sep 10, 2018
@tpendragon
Copy link
Contributor

@Cam156 We'd like to do this in the maintenance sprint next week (week of 9/17). Are there other tickets we should work on to enable the deprecation of Solrizer? README updates, etc? Do you have a sense (or maybe @afred?) about where the best place to put the deprecation warning would be - IE, on gem install vs calling certain Solrizer methods?

@afred
Copy link
Author

afred commented Sep 11, 2018

I'd say put a deprecation warning on the installation, directly above the module declaration (so it's triggered when you first require 'solrizer', and it might not hurt to put one on Solrizer.solr_name, which I'm guessing is how it is most frequently used.

@afred
Copy link
Author

afred commented Dec 18, 2018

It was my understanding that using the Solrizer gem was slated to become a deprecated practice. If that's not the case, then I agree, don't issue deprecation warnings. But if it is the case that we want to discourage use of the solrizer gem, then where else should we be issuing messages?

@jcoyne
Copy link
Contributor

jcoyne commented Dec 18, 2018

It's deprecated (as in we discourage its use) in modern versions of Hyrax/HydraHead, but I still use it with my very old Hydra-Head (7.x) applications. I'd prefer not to see deprecation warnings there, because it's perfectly reasonable to use it in an old Hydra-head.

@afred
Copy link
Author

afred commented Dec 18, 2018

Cool. I guess I misunderstood the fate of Solrizer. I assumed we'd soon stop providing support, but I guess that's incorrect?

I'm fine closing this ticket as well as the PR that would close it. But what's then the best way to discourage it's use in Hyrax? For instance, what prompted me to create this ticket in the first place was that our Hyrax app we were using Solrizer.solr_name in our indexers. If that's discouraged, is there a way we can discourage it programatically, so that programmers who may have missed it in the release notes can be made aware in code?

@jcoyne
Copy link
Contributor

jcoyne commented Dec 18, 2018

@afred I think we do stop providing support as in "we're not going to continue to develop this further", I just don't want to degrade the existing experience.

@afred
Copy link
Author

afred commented Dec 18, 2018

I understand not wanting to throw a bunch of deprecation warnings if the impelmenters intent is to keep using Solrizer gem.

But what happened in our case was that we were using Solrizer.solr_name in our indexers, and then after an upgrade suddenly started getting uninitialized constant Solrizer after an upgrade, because it was no longer part of the bundle.

This is the type of error I think we should be responsible for providing information on what to do.

The quick fix, in this case, is to just explicitly add solrizer to your bundle. But that's actually not the recommended practice.

So I'm just trying to figure out what is the best way to encourage best practice, and avoid head scratching errors on upgrades. The answer very well could be simply to make sure to put something in the release notes. But it's always nicer, imo, to get hints from things like deprecation messages, if possible.

@jcoyne
Copy link
Contributor

jcoyne commented Dec 18, 2018

@afred you would never get that unless you have a dependency on solrizer and you are not explicitly including it in your bundle. The same thing could happen if you depend on some other library that is bundled by another dependency (say Faraday or Nokogiri). You should just adjust your Gemfile so that you list out all of your dependencies that you use explicitly.

@afred
Copy link
Author

afred commented Dec 18, 2018

It's slightly different though, because both Solrizer and ActiveFedora are both managed by the Samvera community. At the same time, it was recommended practice to use Solrizer.solr_name in your indexers (or at least there were lots of examples showing that's how to do it), and then we changed the recommendation to not do that.

I'm just looking for the best solution that programmers can do to help implementers get on the same page about what the best practice is.

And again, maybe that solution is to just really emphasize the release notes.

@jcoyne
Copy link
Contributor

jcoyne commented Dec 18, 2018

There's nothing stopping you from using Solrizer if you want to. If you use a library you need to declare it in your Gemfile. So, this seems like it could be fixed by adding an upgrade note like:

Solrizer is no longer a dependency of Hyrax. Add gem 'solrizer' to your gemfile if you want to continue to use solrizer

@mark-dce
Copy link
Contributor

FYI - I've proposed deprecation of the gem entirely. I kind of agree that older installations still requiring it probably don't benefit from code-level deprecation warnings.

See #55
And https://groups.google.com/forum/#!topic/samvera-tech/T83tGB2Sypo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants