search

sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
951 stars 405 forks source link

bot: properly deprecate `search_url_callbacks()` method #2581

Closed dgw closed 6 months ago

dgw commented 7 months ago

Description

That decorator is the only thing wot might actually get people to notice that they're still using a deprecated feature…

Carelessly left out of #2121, somehow. Actually was included in #2121 but reverted in #2156 due to log spam. But it's not so important that it needs to go in 8.0.0, either.

Checklist

dgw commented 6 months ago

I might have figured out why #2121 didn't do this: url still needs to call this method.

https://github.com/sopel-irc/sopel/blob/120477fe28f470f9308163203c43688ff9121e12/sopel/builtins/url.py#L503-L507

What do y'all think of adding this warning_in 8.1 as planned, and then having url cease checking the deprecated URL callback machinery in 8.1?

The best alternative I could imagine is making the public search_url_callbacks() method a deprecated-decorated wrapper around a private method that isn't decorated, so url could continue checking the old machinery without a warning until it's actually removed in 9.0. That feels messier than I'd like, though.

SnoopJ commented 6 months ago

I think it's okay to formally deprecate it in 8.0.0 and defer the warning. It should obviously be mentioned in the change notes, but deferring the warning to 8.1.0 because we're using it internally makes sense to me. There should be an issue tracking the internal change to url, though. (I didn't check if there already is one)

dgw commented 6 months ago

Closed the loop on this being "forgotten" in #2121 (it wasn't) when I noticed #2156 in the changelog draft @SnoopJ put together. I won't merge this until after I have a moment to create a draft PR implementing one of the solutions in https://github.com/sopel-irc/sopel/pull/2581#issuecomment-1847976882 for 8.1.0, so we don't forget about it and have a repeat occurrence of logspam.

dgw commented 6 months ago

2585 for follow-up; I chose a method that would let url continue interacting with anything added via the deprecated system without additional warnings.

Will merge this soon, once other maintainers have a chance to voice objection.

Edit: "Soon" = 14 Dec, 3 days later. I'm sure any objections would've come up sooner. 🙃