core: handle URL callbacks in coretasks #1510

Exirel · 2019-03-21T20:32:28Z

This fix #1489 - at last!

PR #1508 added a new interface for plugin maintainers, that would help us work with URL callbacks. In this PR, I:

never trigger URL Callbacks from the url built-in plugin
share the "find URL" feature into sopel.web (with unit tests for good measure)
trigger URL callbacks from a @url defined in coretasks

This way, one may disable the url plugin, and the bot won't auto-title URLs. However, other plugins will still be able to define and use URL Callbacks.

Plugins that use @rule and define their own callback will still work, and that won't trigger callbacks.

By the way, this should fix sopel-irc/sopel-github#13 too as soon as the github plugin uses @url instead of @rule.

dgw

Some little notes here and there, but I have a bigger question.

If search_urls (almost) always gets used wrapped in list(), why does it not just return a list in the first place? That's probably more intuitive from an API standpoint than the yield/generator pattern. Even excluding the tests, more uses of search_urls want a list than not.

There are ways to keep the generator-style algorithm and still return a list, FWIW.

sopel/coretasks.py

sopel/modules/url.py

test/test_web.py

sopel/web.py

@Exirel

A change that should have been made in #1413, but was overlooked. This proves why having more than one developer review changes is useful. @Exirel indirectly found this omission during an overhaul of Sopel's URL-handling for version 7. See #1510 (comment)

Exirel · 2019-03-23T08:45:53Z

@dgw I think I fixed most of the issue, you're welcome for the second round, and once everything's fine I'll rebase/squash accordingly. :)

Exirel · 2019-03-23T14:14:43Z

multiple spaces before operator

Damn, I used flake8 sopel but forgot the test-suite. I was quite in a hurry this morning, my bad!

test/test_web.py

Exirel · 2019-03-23T23:14:58Z

What the hell is wrong with Python 2.7 this time???

Edit: that's what:

- if url[-1] is closer and url.count(opener) < url.count(closer):
+ if url[-1] == closer and url.count(opener) < url.count(closer):

Damn you, Py2.7. 😠 💢 💢 💢

dgw · 2019-03-23T23:48:52Z

Is that really an issue in py2? If so it probably needs to be applied on 6.6.x too :/

Exirel · 2019-03-23T23:58:07Z

Is that really an issue in py2? If so it probably needs to be applied on 6.6.x too :/

Yes. 😭

dgw · 2019-03-24T00:14:44Z

Is that really an issue in py2? If so it probably needs to be applied on 6.6.x too :/

Yes. 😭

Well. You'd think I would have caught that myself, given that my primary Sopel instance still runs in py2 and I'm therefore invested in maintaining compatibility…

Added this bugfix to my todo list for 6.6.x. :/

@Exirel

A change that should have been made in sopel-irc#1413, but was overlooked. This proves why having more than one developer review changes is useful. @Exirel indirectly found this omission during an overhaul of Sopel's URL-handling for version 7. See sopel-irc#1510 (comment)

Exirel · 2019-03-27T21:44:37Z

@dgw From my point of view, everything's fine. Yet it appears that Travis isn't happy with an unrelated test (it has something to do with Google stuff).

So if you want to merge, it's ready for the last review.

Exirel · 2019-03-27T22:26:00Z

Py2.7 & Py3.4 tests pass with the exception of the ip test, so I think we are good. :)

dgw · 2019-03-27T22:31:03Z

I'll get Travis to re-run the tests after #1523 makes it into master. Did you want to do any squashing on this branch at all, @Exirel, or just leave all the commits in? (I'm fine with having the full evolution, just curious if you planned to squash down the little fixups like for flake8 failures and py2.7 compat etc.)

Stale review, and I need a "dismissed" webhook payload to examine for sopel-github dev reasons

dgw

I know you're planning to change some more stuff in here, @Exirel, but I noticed a thing or two that jumped out at me and then decided to just do a review because why not? 😁 Admittedly I skimmed over some things that I reviewed before, so hopefully I didn't miss anything because I thought I already looked at it but didn't.

Thought about suggesting to put the URL callback handler in bot.py instead of coretasks.py, but… nah. It doesn't belong there.

sopel/coretasks.py

sopel/modules/url.py

sopel/web.py

test/test_web.py

dgw · 2019-04-15T20:08:36Z

Shall we get back on this horse now that you have a laptop again, @Exirel? 😁

Exirel · 2019-04-15T21:59:40Z

Oh yes, right. I'll resume working on that very soon.

dgw

I had to rush this review, but I wanted to make sure it got done. The rest of my evening could extend way past the point when I'd be any use at code review.

My only real question is why the url module (plugin? we should talk about whether to move sopel.module decorators to sopel.plugin in 8 or 9+) still uses @module.rule to look for URLs, when it could instead use the URL callbacks. This is to prevent triggering on URLs that are handled by any other modules, right?

Seems sad that we (you) go to all this work building a nice URL-callback system that actually works, only for it to be unusable in the module literally named "url". 🤣 But we talked about this before. It's that damn threading problem—modules can't block each other from running, so instead url has to check if any other handler exists for the link it's about to process and stop itself.

Actually, would it be excessively messy for url to use a URL callback just like everyone else, but abort early if any other callbacks match the URL it gets? (Probably, but I have to ask.)

sopel/config/core_section.py

sopel/web.py

Exirel · 2019-04-20T06:48:54Z

@dgw I manually tested that everything worked fine. Now I'll try to write some documentation. This can be done in this PR, or in a separated one. As you wish. :)

Exirel · 2019-04-20T07:49:47Z

I added more information in the sopel.module.url decorator's docstring, with a link to auto_url_schemes and to bot.register_url_callback.

Note that I've tested the example added in this docstring to make sure it worked as intented.

dgw

Two itsy bitsy docstring tweaks, and I think we're there!

I'm going to be lazy and not test this very exhaustively, because I trust your testing regimen better than my own (and you wrote thorough unit tests, too). 😛

sopel/config/core_section.py

Exirel · 2019-04-21T13:20:16Z

@dgw ready for a last review to 🚢

dgw

(optionally after squashing any fixup commits you'd like to clean from the history) 🎉

* take advantage of generator function * generate tinyurl for valid URL only * store **valid** last seen and processed URL, not the last found

Exirel · 2019-04-23T21:34:47Z

Fixup commits squashed, ready to 🚢

@Exirel

A change that should have been made in sopel-irc#1413, but was overlooked. This proves why having more than one developer review changes is useful. @Exirel indirectly found this omission during an overhaul of Sopel's URL-handling for version 7. See sopel-irc#1510 (comment)

Exirel added High Priority Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Mar 21, 2019

Exirel added this to the 7.0.0 milestone Mar 21, 2019

Exirel self-assigned this Mar 21, 2019

Exirel requested a review from dgw March 21, 2019 20:32

dgw previously requested changes Mar 22, 2019

View reviewed changes

sopel/coretasks.py Show resolved Hide resolved

sopel/modules/url.py Outdated Show resolved Hide resolved

test/test_web.py Show resolved Hide resolved

sopel/web.py Outdated Show resolved Hide resolved

dgw mentioned this pull request Mar 22, 2019

url: fixing private URL lookups #1439

Merged

dgw mentioned this pull request Mar 22, 2019

url: Actually clean URLs when auto-fetching titles #1515

Merged

Exirel commented Mar 23, 2019

View reviewed changes

test/test_web.py Outdated Show resolved Hide resolved

dgw mentioned this pull request Mar 24, 2019

url: Fix py2 compatibility in trim_url() #1517

Merged

This was referenced Mar 26, 2019

Trailing punctuation on GitHub URLs isn't ignored sopel-irc/sopel-github#7

Closed

Disabling url.py module also disables @url decorator #1489

Closed

Exirel force-pushed the core-url-into-coretasks branch from 5df9a49 to c19ebeb Compare March 27, 2019 20:26

Exirel force-pushed the core-url-into-coretasks branch from 7431ef4 to 349f4e0 Compare March 27, 2019 22:37

dgw self-requested a review March 27, 2019 23:29

dgw reviewed Mar 28, 2019

View reviewed changes

dgw requested changes Apr 19, 2019

View reviewed changes

sopel/config/core_section.py Outdated Show resolved Hide resolved

sopel/config/core_section.py Outdated Show resolved Hide resolved

sopel/config/core_section.py Outdated Show resolved Hide resolved

sopel/web.py Show resolved Hide resolved

dgw removed the Needs Review label Apr 20, 2019

Exirel force-pushed the core-url-into-coretasks branch 2 times, most recently from 56b8523 to 3317c0b Compare April 20, 2019 06:37

dgw reviewed Apr 21, 2019

View reviewed changes

sopel/config/core_section.py Outdated Show resolved Hide resolved

sopel/config/core_section.py Outdated Show resolved Hide resolved

dgw approved these changes Apr 22, 2019

View reviewed changes

Exirel added 14 commits April 23, 2019 23:33

url: never trigger bot's URL callbacks

62f00ca

url: replace find_url by sopel.web.search_urls

585472f

core: trigger bot's URL callbacks as a core feature fix sopel-irc#1489

1c91e37

web: extract and test trim_url

3f8c07f

url: clean URL for auto_title

a90d05c

test: py2.7 compatibility on unicode comparison

fd78126

url: initialize shortened_urls in setup()

ac4b081

url: improved URL processing

a2819e8

* take advantage of generator function * generate tinyurl for valid URL only * store **valid** last seen and processed URL, not the last found

url: organize imports

d462ecf

url: remove trigger parameter from check_callbacks

806f296

url: remove limitation on number of URLs processed

22168ce

url: always store last seen URL

110a8a0

core: configure schemes to trigger URL Callbacks on

e69a235

module: document the sopel.module.url decorator

b3ac099

Exirel force-pushed the core-url-into-coretasks branch from 0b9ec40 to b3ac099 Compare April 23, 2019 21:33

dgw merged commit 6e5ded2 into sopel-irc:master Apr 24, 2019

Exirel deleted the core-url-into-coretasks branch September 5, 2019 09:44

dgw mentioned this pull request Jun 23, 2020

https://freebies.indiegala.com doesn't show any results from Sopel bot. #1895

Closed

dgw mentioned this pull request Sep 29, 2020

Error messages were removed from .title command #1946

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: handle URL callbacks in coretasks #1510

core: handle URL callbacks in coretasks #1510

Exirel commented Mar 21, 2019

dgw left a comment

Exirel commented Mar 23, 2019

Exirel commented Mar 23, 2019

Exirel commented Mar 23, 2019 •

edited

Loading

dgw commented Mar 23, 2019

Exirel commented Mar 23, 2019

dgw commented Mar 24, 2019

Exirel commented Mar 27, 2019

Exirel commented Mar 27, 2019

dgw commented Mar 27, 2019

dgw left a comment

dgw commented Apr 15, 2019

Exirel commented Apr 15, 2019

dgw left a comment

Exirel commented Apr 20, 2019

Exirel commented Apr 20, 2019

dgw left a comment

Exirel commented Apr 21, 2019

dgw left a comment

Exirel commented Apr 23, 2019

core: handle URL callbacks in coretasks #1510

core: handle URL callbacks in coretasks #1510

Conversation

Exirel commented Mar 21, 2019

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Mar 23, 2019

Exirel commented Mar 23, 2019

Exirel commented Mar 23, 2019 • edited Loading

dgw commented Mar 23, 2019

Exirel commented Mar 23, 2019

dgw commented Mar 24, 2019

Exirel commented Mar 27, 2019

Exirel commented Mar 27, 2019

dgw commented Mar 27, 2019

dgw left a comment

Choose a reason for hiding this comment

dgw commented Apr 15, 2019

Exirel commented Apr 15, 2019

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Apr 20, 2019

Exirel commented Apr 20, 2019

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Apr 21, 2019

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Apr 23, 2019

Exirel commented Mar 23, 2019 •

edited

Loading