irc: fix bot.say() handling of truncation & trailing args
#2450
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In certain cases (such as `max_messages=1` with no `trailing` or `truncation` parameters, i.e. how most `bot.say()` calls are written), Sopel seems to have been skipping the message truncation step, as evidenced by manual test results involving strings of emoji that the IRC server would further truncate in the middle of UTF-8 sequences (given the correct-length test bot nick/hostmask). Length values calculated by the tests in question (used for building test strings dynamically instead of hard-coding 400-plus-byte literals) only took into account the size of the PRIVMSG *sent by Sopel*, without the hostmask prefix added by the IRC server when forwarding the message to other clients. This is a lesson in making sure the tests are correct, because a passing test case means nothing if what it checks is wrong.
dgw
force-pushed
the
bot.say-fix-safe-length
branch
from
a4b4ef2 to
43baee1
Compare
Exirel
approved these changes
May 3, 2023
SnoopJ
approved these changes
May 3, 2023
Contributor
|
@dgw (poke) does this need anything to be merged? |
Member
Author
|
@SnoopJ Usually that's something the PR author asks reviewers, but y'all already approved this one 😁 I'll get to it through the week, while I avoid Real Work™ on PRs that aren't approved yet in favor of meeting $work deadlines. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Some use cases involving
bot.say()(text with no truncation and no continuation, i.e. the majority of that method's uses throughout core and third-party plugins alike) did not truncate long text correctly.It seems that incorrectly written tests passed anyway, letting the incorrect behavior appear as if it was correct.
I've verified that without this change, #2314 was still reproducible using the
sopel-ipythonconsole plugin on e5bcd1f (masteras of writing); and with this change, I could no longer reproduce that behavior.Checklist
make qa(runsmake qualityandmake test)Notes
This PR was opened early because I'm a cheeky bastard and want to make number 2450 a PR that we'd definitely ship.