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

url: fix premature loop termination, docstring cleanup & slight refactoring #2433

Merged
merged 2 commits into from
Mar 28, 2023
Merged

url: fix premature loop termination, docstring cleanup & slight refactoring #2433

merged 2 commits into from
Mar 28, 2023

Conversation

SnoopJ
Copy link
Contributor

@SnoopJ SnoopJ commented Mar 25, 2023

Description

This changeset resolves #2230 by dropping the incorrect information from the relevant docstrings, and also does some light refactoring of title_auto(), process_urls() for the sake of future maintenance. Also included is a fix for a bug discovered while writing this PR, where a URL matching a registered callback causes following URLs in the message to be ignored.

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 quality and make test)
    • 1426 passed, 8 xfailed, 1 warning in 52.07s
  • I have tested the functionality of the things this change touches

@SnoopJ SnoopJ added this to the 8.0.0 milestone Mar 25, 2023
@SnoopJ SnoopJ requested a review from a team March 25, 2023 05:25
Exirel
Exirel requested changes Mar 25, 2023
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.

Small nitpick here and there.

sopel/modules/url.py Show resolved Hide resolved
sopel/modules/url.py Show resolved Hide resolved
sopel/modules/url.py Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
dgw
dgw requested changes Mar 26, 2023
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.

Some Mostly nitpicks. When you've addressed these, go ahead and re-request review from both me and Exi.

sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
sopel/modules/url.py Outdated Show resolved Hide resolved
@dgw dgw added Documentation Bugfix Generally, PRs that reference (and fix) one or more issue(s) Housekeeping Code cleanup, removal of deprecated stuff, etc. labels Mar 26, 2023
@dgw dgw changed the title Fix-up incorrect docstrings in url url: fix premature loop termination, docstring cleanup & slight refactoring Mar 26, 2023
@SnoopJ SnoopJ requested review from Exirel and dgw March 26, 2023 22:23
dgw
dgw approved these changes Mar 27, 2023
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.

Just a side note, "doom and gloom" for the future of one url plugin subfeature that I doubt gets much use (but we don't collect usage metrics, so who knows?)

sopel/modules/url.py Show resolved Hide resolved
SnoopJ and others added 2 commits March 28, 2023 11:06
@SnoopJ
url: fix premature loop termination
42dff14
@SnoopJ @Exirel @dgw
url: clarify redirection behavior, tidy up docs
2051f95 
Co-authored-by: Florian Strzelecki <florian.strzelecki@gmail.com>
Co-authored-by: dgw <dgw@technobabbl.es>
@SnoopJ SnoopJ force-pushed the chore/gh2230-tidy-up branch from f2aba3d to 2051f95 Compare March 28, 2023 15:09
Exirel
Exirel approved these changes Mar 28, 2023
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.

I haven't tested this myself, however it looks sane to me.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Mar 28, 2023 •
edited
Loading

Re: testing, I showed off on IRC that with this patch, [url] does not swallow URLs that come after one handled by some other plugin:

13:06 <+SnoopJ> https://github.com/sopel-irc/sopel/ https://arxiv.org/abs/2303.12132
13:06 <+testibot> [url] [2303.12132] Fundamentals of Generative Large Language Models and Perspectives in Cyber-Defense | arxiv.org
13:06 <+testibot> [GitHub] sopel-irc/sopel - :robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie. | 99.5% Python 0.4% Shell 0.1% Makefile | Last Push: 2023-03-28 - 15:09:05UTC | Stargazers: 924 | Watchers: 65 | Forks: 414 | Network: 414 | Open Issues: 95
13:06 <+Sopel> [GitHub] sopel-irc/sopel - :robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie. | 99.5% Python 0.4% Shell 0.1% Makefile | Last Push: 2023-03-28 - 15:09:05UTC | Stargazers: 924 | Watchers: 65 | Forks: 414 | Network: 414 | Open Issues: 95

@dgw dgw merged commit 9497c45 into sopel-irc:master Mar 28, 2023
@SnoopJ SnoopJ deleted the chore/gh2230-tidy-up branch March 28, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Exirel Exirel Exirel approved these changes

@dgw dgw dgw approved these changes

Assignees
No one assigned
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Documentation Housekeeping Code cleanup, removal of deprecated stuff, etc.
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

url: documented as calling other plugins when redirects match, but doesn't
3 participants
@SnoopJ @dgw @Exirel