rsolr-ext
rsolr-ext copied to clipboard
mwmitchell
deprecation warning on response.docs
I see what you were trying to do, but this deprecation warning is really annoying. Every time I call response.docs, I get told:
warn "DEPRECATION WARNING: The custom pagination codebase in RSolr::Ext will no longer be supported. Use response.docs.will_paginate instead."
You see the problem here is that what it's suggesting I do instead is STILL calling response.docs! Calling response.docs.will_paginate will output the deprecation warning too. So even if I do what it's telling me to do, I still get the deprecation warning. And in fact, I get the deprecation warning on any call to response.docs, but the #docs method itself is NOT what you intend to deprecate. So there's no way to get rid of the deprecation warning. And it shows up all over the place in my logs, and in my output when running tests. It's really problematic.
Is there a better way to do this please? Including not outputting the deprecation warning. A deprecation warning that outputs even when you are NOT doing anything deprecated is just annoying.
+1 -- this is very problematic for readability of unit and integration tests, which end up swamped with the deprecation warnings.
PS: Personally, will_paginate broke in Rails 3.1 too. (generally not RSolr specific) Personally I'm leaning towards thinking will_paginate is a mess and not worth spending time on trying to duck-type to work with it.
Kaminari, which some people seem to be using as a replacement... kind of suggests to the developer that it's not worth using kaminari unless you're using ActiveRecord, otherwise you're over-riding too much for too little, just implement it yourself. I think it may not be worth it for RSolr to try and do any of this, honestly.
This is seriously making my test output unreadable and useless. I'm going to have to have Blacklight monkey patch to get rid of the deprecation warning unless you release another version that does. Thoughts?
Hey Matt, if you can give me the list of methods that actually ought to be deprecated (ie, if you call this method you're doing something deprecated), I'll try to prepare a pull request that removes the deprecation warning every time you call response.docs (the #docs method is not deprecated!), and instead issues it when you actually call a deprecated method.
Otherwise I'll prepare a pull request taht simply removes the deprecation warning, the end.
Otherwise, if you're not interested in such, I'll have to figure out how to have Blacklight monkey-patch RSolr::Ext to remove the deprecation warning. It's not acceptable to have a deprecation warning when the client is not in fact doing anything deprecated, even less so when I get several hundred of them in my console output every time I run blacklight automated tests, intefering with my ability to understand automated test output.
Sorry guys. I didn't see a notification about this issue. I originally had the deprecation warning at the top of the file, but someone else didn't like that solution. I'm open to whatever it is people need, so let me see if I can either add the warnings to each pagination method, or send you a list Jonathan.
Yeah, the general principle about deprecation warnings that seems
obvious to me, is a deprecation warning should be output if and ONLY
if the client code is actually using deprecated functionality. So top
of the file is no good either, it's got to be triggered by the actual
deprecated behavior, or it's just a false alarm/crying wolf/not a real
deprecation warning. General notes about what to change go in docs --
it doesn't go in a deprecation warning output to logs at runtime
unless it's warning you that you are actually using deprecated
functionality.
On Jun 6, 2011, at 6:57 PM, mwmitchell wrote:
Sorry guys. I didn't see a notification about this issue. I
originally had the deprecation warning at the top of the file, but
someone else didn't like that solution. I'm open to whatever it is
people need, so let me see if I can either add the warnings to each
pagination method, or send you a list Jonathan.Reply to this email directly or view it on GitHub: https://github.com/mwmitchell/rsolr-ext/issues/14#comment_1312517
Yep yep, makes sense. I've added warnings to all methods in the Pagination module. I'm fine with removing it, not sure if others will agree?
Thanks! Sorry, you're not sure if others will agree with what?
Removing the deprecation warning on #docs? I am pretty positive
nobody will have a problem with that, if they do tell em it's my
fault, heh. But i mean, deprecation warnings only if you do deprecated
things and not if you do non-deprecated things (like calling #docs),
who could disagree with that?
If you mean removing the deprecated methods (or if they should be
deprecated in the first place), well, yeah, some people might want em
to stick around for a bit (or forever). That's another story.
I'd love another point release that only issues deprecation warnings
(at least in this area) if one is doing deprecated behavior, so one
can get rid of them in one's logs by ceasing to do deprecated things.
On Jun 6, 2011, at 10:16 PM, mwmitchell wrote:
Yep yep, makes sense. I've added warnings to all methods in the
Pagination module. I'm fine with removing it, not sure if others
will agree?Reply to this email directly or view it on GitHub: https://github.com/mwmitchell/rsolr-ext/issues/14#comment_1313214
Oh sorry. I mean if anyone else will be bothered with removing pagination all together? I'm OK with that, but need to think a little more on that. Supporting will_paginate directly with a will_paginate method on the docs array is OK, but that means being tied to will_paginate, and if it's dying then hmm. The generic pagination module is somewhat useful actually, since it's really just doing some simple pagination math and requires no dependencies.
I definitely agree with what you're saying about warnings. I've made those changes.
Just need to think more on whether or not pagination stays, or goes all together.
Yeah, I'm not sure about that either. But maybe a patch release with
fixed deprecations anyway while we wait?
But background, for consideration, me and cbeer are thinking about
ending hte use of will_paginate in default Blacklight, sooner rather
than later. In part because will_paginate doesn't (yet?) work with
Rails 3.1 from the official distro, and in part becaues will_paginate
in general is just a mess to work with, doesn't have a public API for
over-riding the things we want to over-ride or work with, leading to
this RSolr confusion etc. We are considering a kaminari adapter for
RSolr (which could be provided in Blacklight, or in a seperate gem,
not neccesarily in RSolr::Ext); we are considering just pagination
homegrown in BL.
Oh, but I guess then there is one thing I agree with: I agree RSolr
should provide it's own methods sufficient to know what you need to
know for pagination. That are not deprecated, that are just part of
RSolr. Looking at the current page, the total number of hits/pages,
the per_page, etc. That is useful to have in RSolr, so people CAN
build pagination in their app however they want. In fact, it's so
useful, I wonder if it could be in RSolr itself instead of
RSolr::Ext? So please don't take that out, I'd leave that in -- and
in non-deprecated methods, just not in methods that are documented/
promised to work with any particular moving-target external library
like will_paginate or even kaminari.
On Jun 6, 2011, at 10:37 PM, mwmitchell wrote:
Oh sorry. I mean if anyone else will be bothered with removing
pagination all together? I'm OK with that, but need to think a
little more on that. Supporting will_paginate directly with a
will_paginate method on the docs array is OK, but that means being
tied to will_paginate, and if it's dying then hmm. The generic
pagination module is somewhat useful actually, since it's really
just doing some simple pagination math and requires no dependencies.I definitely agree with what you're saying about warnings. I've made
those changes.Just need to think more on whether or not pagination stays, or goes
all together.Reply to this email directly or view it on GitHub: https://github.com/mwmitchell/rsolr-ext/issues/14#comment_1313262
I agree completely. I've skimmed the will_paginate codebase a few times and have wondered what was going on -- not to say that I don't look at my own code and scratch my head. So maybe toss the will_paginate support, then keep the generic pagination module in rsolr. Sounds good to me. I will definitely do a patch release for the warnings fix.
I have removed the warnings from rsolr and rsolr-ext, and have released new versions of both libraries. Generic pagination methods will stay in rsolr, and the next release of rsolr-ext will piggyback those methods. The pagination methods will not try to match any specific library interface (will_paginate), they'll just be useful helpers like current_page, has_next? etc.. Pretty much what we have now, except rsolr-ext will have a bunch of pagination code removed, and use rsolr instead.
Anything else we need to cover?
Sounds good to me, I think. Thanks!
On Wed, Jun 8, 2011 at 11:21 PM, mwmitchell [email protected] wrote:
I have removed the warnings from rsolr and rsolr-ext, and have released new versions of both libraries. Generic pagination methods will stay in rsolr, and the next release of rsolr-ext will piggyback those methods. The pagination methods will not try to match any specific library interface (will_paginate), they'll just be useful helpers like current_page, has_next? etc.. Pretty much what we have now, except rsolr-ext will have a bunch of pagination code removed, and use rsolr instead.
Anything else we need to cover?
Reply to this email directly or view it on GitHub: https://github.com/mwmitchell/rsolr-ext/issues/14#comment_1331605