Skip to content

Conversation

@half-duplex
Copy link
Member

@half-duplex half-duplex commented Jan 17, 2020

Description

Implements #1804

This introduces Sopel._url_callbacks, changes the *_url_callback functions to use it, and returns both Sopel._url_callbacks and bot.memory["url_callbacks"] results in a search, maintaining the legacy functionality.

Because Sopel doesn't have a SopelMemory-type object for lists, this may have minor concurrency issues, but url_callbacks updates are infrequent enough I don't think it's deadly.

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 force-pushed the multiple-url-callbacks branch from e3cffe6 to 5fd9823 Compare January 18, 2020 00:01
@half-duplex half-duplex added this to the 8.0.0 milestone Jan 18, 2020
@half-duplex half-duplex force-pushed the multiple-url-callbacks branch from 5fd9823 to ad1530c Compare January 18, 2020 03:26
@Exirel
Copy link
Contributor

Exirel commented Jan 21, 2020

I like the idea, and the code is quite good. Yet, I'd like to point out that we planned on removing memory['url_callbacks'], to prevent any manual access to this feature - as mentioned in the issue:

At the same time, we can officially remove the memory dict.

For that, I think you can add a new attribute to Sopel, something like Sopel.url_callbacks maybe? It could be a SopelMemory object, where keys are compiled regex. Alternatively, it could be a protected attribute, like _url_callbacks.

@half-duplex half-duplex force-pushed the multiple-url-callbacks branch from ad1530c to d1d5c2a Compare January 22, 2020 01:43
@half-duplex half-duplex modified the milestones: 8.0.0, 7.1.0 Jan 22, 2020
@half-duplex half-duplex force-pushed the multiple-url-callbacks branch 2 times, most recently from 778bbb0 to 77a4ada Compare January 22, 2020 03:31
@half-duplex
Copy link
Member Author

Thanks. Yeah, this was originally aimed for 7.0 which needs to keep manual management. Mostly redone now to be fully compatible with no API changes at all, so this maintains exact compatibility with the manual memory["url_callbacks"] way.
I like it more as a private attribute, since I can't think of any valid reason to directly work with it from a plugin, and when private we don't need to worry about compatibility if it requires any further 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.

I like it. Few nitpicks to consider.

@dgw
Copy link
Member

dgw commented May 12, 2020

I'm in favor of doing this for 7.1. Where are we on addressing Exi's feedback?

@half-duplex half-duplex force-pushed the multiple-url-callbacks branch from 3704686 to 844745f Compare May 18, 2020 18:48
@half-duplex half-duplex requested a review from Exirel May 18, 2020 20:49
if 'url_callbacks' not in self.memory:
# nothing to search
return

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the line here? I like the code with a bit of space, for readability.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was to make clear that the whole block should be removed in 8.0 per the above comment. I'm fine with or without the blank line here, but I understand the (suspected) reasoning behind deleting it.

self.memory['url_callbacks'][pattern] = callback
if pattern not in self._url_callbacks:
self._url_callbacks[pattern] = []
self._url_callbacks[pattern].append(callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should check if callback is not yet registered for that pattern.

del self.memory['url_callbacks'][pattern]
except KeyError:
self._url_callbacks[pattern].remove(callback)
except ValueError:
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to catch a KeyError here too.

@dgw
Copy link
Member

dgw commented Jul 11, 2020

I suspect #1904 will replace this. Even if not, this will need some refactoring after the recent massive changes to how plugin rules work. @Exirel confirmed so on IRC.

@dgw dgw closed this Jul 11, 2020
@dgw dgw added the Replaced Superseded by a newer PR label Jul 11, 2020
@dgw dgw removed this from the 7.1.0 milestone Jul 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Replaced Superseded by a newer PR Tweak

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants