trigger: Make sender property more consistent
#2359
Merged
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
Short sleep does not make me a great comprehensive reader of specs, but commands that are 1) sent from the server to a client and 2) include a <target> (channel/nick), are actually quite rare. The IRC protocol is mostly numeric replies, and very few event types actually refer to a user (nick) or channel. The idea here, though, is that unless I forgot about an event type that actually is tied to a channel or query (PM) buffer, it is ONLY the new list of `COMMANDS_WITH_CONTEXT` that should have the `sender` set to any value other than `None`. Haven't yet addressed the fact that `sender` is not the clearest name for this property, though. Baby steps. While updating/adding tests I also found an actual bug in how `text` gets set (it contradicts Sopel's documentation). TODO comments related.
dgw
added
the
Bugfix
dgw
added
the
Breaking Change
Exirel
approved these changes
Oct 13, 2022
Contributor
Exirel
left a comment
Exirel
left a comment
There was a problem hiding this comment.
I'm impressed. It's actually quite good: logic is good, code is sane, there are tests! All good to me.
4 tasks
SnoopJ
reviewed
Oct 13, 2022
SnoopJ
reviewed
Oct 13, 2022
Micro-optimization suggested in code review. Co-authored-by: James Gerity <[email protected]>
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
Short sleep does not make me a great comprehensive reader of specs, but commands that are 1) sent from the server to a client and 2) include a (channel/nick), are actually quite rare. The IRC protocol is mostly numeric replies, and very few event types actually refer to a user (nick) or channel.
The idea here, though, is that unless I forgot about an event type that actually is tied to a channel or query (PM) buffer, it is ONLY the new list of
COMMANDS_WITH_CONTEXTthat should have thesenderset to any value other thanNone.Checklist
make qa(runsmake qualityandmake test)Notes
Undecided yet if this is enough to call #2331 closed. I remember a discussion about transitioning to a new property called
trigger.contextor similar, for clarity, but can't find it on the relevant issue. Maybe it was an IRC conversation.'Breaking Change' label because this could be a spacebar heating incident if anyone relies on the old (buggy) behavior for some weird plugin thing and it needs to be mentioned in the changelog for 8.0.
While updating/adding tests I also found an actual bug in how
textgets set (it contradicts Sopel's documentation). TODO comments related. I will work on that next.