irc: rework parsing of modestrings #2131
Merged
Conversation
This file contains 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
dgw
reviewed
Jul 1, 2021
dgw
reviewed
Jul 8, 2021
Exirel
force-pushed
the
irc-modes-rework
branch
2 times, most recently
from
8dd8d8a
to
178e392
Compare
dgw
reviewed
Jul 18, 2021
|
We are getting close. |
dgw
approved these changes
Sep 18, 2021
There was a problem hiding this comment.
I deem this "close enough", mostly because the best way to find any remaining edge cases (or documentation mistakes) is going to be shipping this and having people use it in production testing environments. Squash the fixups and then bug @half-duplex (who wrote some of the logic this replaces) to take a look if he's willing.
Exirel
force-pushed
the
irc-modes-rework
branch
from
50891c0
to
3fb6cc1
Compare
half-duplex
reviewed
Oct 23, 2021
half-duplex
reviewed
Oct 24, 2021
half-duplex
reviewed
Oct 24, 2021
half-duplex
reviewed
Oct 24, 2021
|
Last round of fixes is available! Hopefully, will be the actual last one. 🤷 |
half-duplex
approved these changes
Oct 28, 2021
@Exirel I think this means you can squash. 🚀 |
Co-authored-by: dgw <dgw@technobabbl.es>
Instead of creating an instance of `ModeParser` at each MODE event, the bot has a `modeparser` attribute with appropriate default values for its chanmodes, privileges, and param types. For that to work, when the bot received new ISUPPORT parameters, it updates the chanmodes and privileges of the mode parser if necessary. This required to adapt the list of default modes, as `a` and `q` are more commonly used as privilege rather than modes. Tests have been updated to manually update the modeparser. This may require new methods on the test IRC server to streamline an ISUPPORT configuration, because right now it's a bit too manual.
Several improvements of the ModeParser.parse method, which was renamed from ModeParser.parse_modestring (renamed to prevent name conflict with sopel.irc.modes.parse_modestring function). The method ModeParser.get_mode_info doesn't try to parse privileges anymore, which make it more type-safe as the letter is now str instead of Optional[str]. This makes the logic in ModeParser.parse a little different in a good way. Overall, a better type definition helped built a better interface with less potential for issues later. Functionnal tests for ModeParser.parse are kept the same, as nothing has change on this side. Thanks to mal who caught the type inconsistency in the first place!
Co-authored-by: mal <mal@sec.gd>
Exirel
force-pushed
the
irc-modes-rework
branch
from
675bcf7
to
ee5dca2
Compare
🚀 landed. |
|
I'm sure CI will fail thanks to #2210, but 4 months in the PR list was enough for this patch. |
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
Following the work on #2098 I was able to refactor that part of the modestring parsing I had in mind since a long time ago. My main goal was to extract the parsing logic, and separate it from the application of new modes & privileges to users.
So now there is a
sopel.irc.modesmodule that contains only modestring parsing (with the appropriate tests), and this module is then used in thecoretasksplugin. There is close to 0 change in behavior, but there is at least two I can remember:deepcopyto get the existing channel modes to prevent any mutability issueschannel.modes = modeswasn't done; this is now fixedNo tests on coretasks have been modified, although I used them locally to ensure that 1. it worked as intended, and 2. without breaking changes. And yes, at first, they didn't pass, and I had to fix what was broken in my code. So I'm pretty happy about all this. As said on IRC, this wasn't possible without the work of @half-duplex that relentlessly fixed the modeparsing before.
One last thing: I used type hint on
sopel.irc.modesand validated it withmypy: so far, so good. I might do more of that later. I just hope it works fine with Py3.6 because I know for a fact that type hinting changed in later versions. Further conversation need to happen around Py3.10 too, because that version might not be easy to support while supporting 3.6.Checklist
make qa(runsmake qualityandmake test)