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

bot: require callback to unregister URL handler #1808

Merged
merged 3 commits into from
Jan 21, 2020
Merged

bot: require callback to unregister URL handler #1808

merged 3 commits into from
Jan 21, 2020

Conversation

half-duplex
Copy link
Member

Description

Introduces the API change #1805 will require. The test diff isn't particularly minimal because this is a reduction of that patchset for 7.0.

Checklist

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

@half-duplex half-duplex added this to the 7.0.0 milestone Jan 18, 2020
@half-duplex half-duplex mentioned this pull request Jan 18, 2020
Closed
4 tasks
@half-duplex half-duplex force-pushed the require-callback-for-unregister branch from 633189a to f6c49d1 Compare January 18, 2020 03:29
dgw
dgw requested changes Jan 18, 2020
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.

Go through the tests again, please. There are test updates that assume behavior changes this patch doesn't implement (that we won't implement until closer to, or part of, 8.0).

test/test_bot.py Outdated Show resolved Hide resolved
test/test_bot.py Outdated Show resolved Hide resolved
@half-duplex half-duplex force-pushed the require-callback-for-unregister branch from 1b79aa9 to 3a0359d Compare January 18, 2020 04:44
@half-duplex half-duplex requested a review from dgw January 18, 2020 05:00
@half-duplex half-duplex force-pushed the require-callback-for-unregister branch 3 times, most recently from 8ee2642 to 69a11af Compare January 18, 2020 05:22
dgw
dgw approved these changes Jan 18, 2020
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. Requesting @Exirel's review for added tests, since he's our current guru.

@dgw dgw requested a review from Exirel January 18, 2020 20:53
Exirel
Exirel approved these changes Jan 18, 2020
View reviewed changes
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Pretty good. One nitpick, more a question of communication to fix than anything else. Good work!

test/test_bot.py Show resolved Hide resolved
@half-duplex half-duplex force-pushed the require-callback-for-unregister branch from 69a11af to bbdaae7 Compare January 19, 2020 07:58
half-duplex and others added 2 commits January 19, 2020 07:58
@half-duplex
bugzilla: specify which callback to unregister
bbdaae7
@dgw
test_tools: update MockSopel's unregister_url_callback signature
05fccb0 
Is it deprecated? Yes.

Is existing code that already uses it likely to call this method? No.

Am I updating it anyway? You bet I am.
@dgw
Copy link
Member

dgw commented Jan 20, 2020

I'm glad of doing one last check in the code…

Highly unlikely that any code actually calls MockSopel.unregister_url_callback(), but let's not allow the signature to be wrong just because the class is deprecated.

@dgw dgw merged commit a4d499f into sopel-irc:master Jan 21, 2020
dgw added a commit that referenced this pull request Jan 22, 2020
@dgw
Merge pull request #1808 from half-duplex/require-callback-for-unregi…
7a80121 
…ster

bot: require callback to unregister URL handler
@dgw
Copy link
Member

dgw commented Jan 22, 2020 •
edited
Loading

Overwrote the original merge commit with a new one after fixing the unrelated ip test failure (presumably caused by a change in MaxMind's latest database version).

I really wish we could put 127.0.0.1 or 192.168.0.1 in the ip test, since those are defined by standards and aren't likely to change. Google keeps playing with its AS and PTR names…

@half-duplex half-duplex deleted the require-callback-for-unregister branch January 22, 2020 01:53
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

@Exirel Exirel Exirel approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@half-duplex @dgw @Exirel