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.

Sign up for GitHub

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

Jump to bottom

sopel: don't use bot.search_url_callbacks internally #2635

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sopel: don't use bot.search_url_callbacks internally #2635

wants to merge 1 commit into from

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Oct 19, 2024

Description

Both coretasks and url used bot.search_url_callbacks, that was deprecated some time ago and wants to emit warning in Sopel 8.1. Because it was used, we didn't bump Sopel's version to 8.1.0.dev0 yet, and that was annoying.

Therefore, I replaced that by a direct access to the private attribute _url_callbacks, and then bumped to Sopel 8.1.0.dev0. And by the way, we should be able to remove bot.call in Sopel 9.

Thank you for your service, register_url_callback, but I can't wait to remove you and your sister methods!

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added the Tweak label Oct 19, 2024
@Exirel Exirel added this to the 8.1.0 milestone Oct 19, 2024
@Exirel Exirel requested a review from a team October 19, 2024 14:56
dgw
dgw reviewed Oct 19, 2024
View reviewed changes
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a glance from my phone, this looks sane. TIL GitHub only shows the "Viewed" checkbox on files in landscape orientation. :/

Thank you for your service, register_url_callback, but I can't wait to remove you and your sister methods!

I was pretty excited about that too… #2436 is biding its time since early last year (but obviously will need a rebase to move out of draft status; I knew the risks going in!)

@Exirel Exirel requested a review from dgw October 19, 2024 18:01
dgw
dgw approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I wish I knew why it took me this long to re-review. 😅

@dgw
Copy link
Member

dgw commented Feb 14, 2025

@Exirel you can squash out the fixup, and drop 4c624ac (master version was updated in 65fabe8 / #2645).

@Exirel
coretasks, url: use bot._url_callbacks
7e46a77 
In order to emit warnings, the method Sopel.search_url_callbacks must
not be used internally or through a built-in plugin. As a result, and
until removal in Sopel 9, both coretasks and url are going to access
bot._url_callbacks directly.

Also, the method `bot.call` is now deprecated and will be removed in
Sopel 9.0, following a short deprecation cycle. This method should have
been deprecated since Sopel 8 but was kept in order to keep the old URL
callbacks system working. When it is gone, this method won't be needed
anymore.
@Exirel Exirel force-pushed the search-url-callbacks-emits-warning branch from 0e3f2bf to 7e46a77 Compare February 15, 2025 09:57
@Exirel
Copy link
Contributor Author

Exirel commented Feb 15, 2025

I squashed against itself, so let's see if the test suite pass properly before merging. I'm still not sure about that whole "don't rebase on master" thing, it feels wrong to me. 🙃

@dgw
Copy link
Member

dgw commented Feb 15, 2025 •
edited
Loading

I'm still not sure about that whole "don't rebase on master" thing, it feels wrong to me

Ah, GitHub runs pull request jobs on the merge result of BASE <- PR_HEAD, so it's almost like rebasing. You still get all of the upstream changes! 😁

This one's checks passed and it's now ready to merge when I get back to my PC in a few days (to do the backport/ changelog stuff properly, very tricky to do on my tablet).

(Edit: No backporting, it's for 8.1. See? Hard to notice all the details on my phone!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgw dgw dgw approved these changes

Assignees
No one assigned
Labels
Tweak
Projects
None yet
Milestone
8.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Exirel @dgw