irc: implement CASEMAPPING parameter for ISUPPORT #2231

Exirel · 2021-12-30T23:48:22Z

Description

This PR contains various changes with the end goal to use the server's casemapping instead of the hardcoded one. Sadly, this includes breaking changes.

Non-breaking changes

The bot has a new method make_identifier: it uses the CASEMAPPING parameter from isupport to select the appropriate casemapping function (from sopel.tools.identifiers)
The Identifier class has several changes:
- Identifier now lives its best life in sopel.tools.identifiers, mostly to prevent cyclic import errors. This comes with some more documentation.
- Identifier doesn't override str.__new__ anymore, I figured out a way to use __init__ instead, and it works fine.
- Identifier has a new argument: casemapping. It's a callable that must take one parameter (a string) and return a string.
- The default value of casemapping is rfc1459_lower, making this argument optional (at the moment).
- self.casemapping is now used in place of Identifier._lower to get a lowercase version of the identifier string
Classes that create Identifier have a new argument: It's a callable that must take one parameter (a string) and return an instance of Identifier with properly configured casemapping.
- User and Channel (both from sopel.tools.target)
- PreTrigger (from sopel.trigger)
- SopelDB (from sopel.db)
- SopelIdentifierMemory (from sopel.tools)
The new argument is used to instantiate Identifier when necessary
coretasks and other modules are now using bot.make_identifier when they need an Identifier
All method from SopelDB now accept a str (some used to accept Identifier only) since it can always convert them into an Identifier when necessary
Various type hint here and there because why not

Breaking changes

Identifier._lower doesn't lower all character as it used to do, now it uses the rfc1459_lower function which lowercase only [A-Z] characters. In theory, this is not a breaking change because IRC identifiers shouldn't accept non-ASCII characters anyway. However, in the case of IRC server supporting Unicode identifiers (for nicknames and/or channels), this is a breaking change. A solution to that would be to implement rfc7613 (PRECIS) as suggested by https://modern.ircdocs.horse/index.html#casemapping-parameter
core.nick is now a str and not an Identifier anymore, meaning that any plugin using that config option will need to transform it into an Identifier; this is considered a breaking change, even tho it shouldn't be a problem considering that most Identifier-related operations are through objects that already convert str into Identifier when necessary

Going further

The next steps would be:

to merge sopel: move deprecated() to sopel.lifecycle #2232
to implement a PRECIS profile for casemapping—I think it would even work on current ascii-only identifier, making Sopel UTF-8 ready before servers can be
to move memory classes into their own submodule
and probably to move other things outside of sopel/tools/__init__.py
to implement dynamic chantype for identifiers

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)
I have tested the functionality of the things this change touches

dgw

I made it through. 😩 Expected to break and finish tomorrow, but nope.

While I'm (understandably?) annoyed by all the extra constructor parameters that had to happen to make this work, I get that it's impossible—or at least, very code-smell-y—to do it another way.

sopel/tools/identifiers.py

docs/source/api.rst

docs/source/plugin/bot.rst

sopel/tools/__init__.py

sopel/tools/time.py

sopel/db.py

sopel/tools/target.py

sopel/trigger.py

Exirel · 2022-01-09T23:11:43Z

I did a rebase and squashed fixup commits. There was some conflicts a bit complicated to sort out properly, I'm glad I made atomic commit so it was easier to test every single one with py.test and flake8 (except for one commit but heh it's squashed now).

I added one commit to update SopelIdentifierMemory's docstring, as requested (I almost forgot about it, so nice catch!).

What's left is:

to decide if you want me to move memory classes into their own submodule of sopel.tools in that PR or not
to write the PR's description, that must contain a list of all the breaking changes and things to consider for the upgrade

I won't write a migration script in that PR, that's too big of a job, and I'm not even sure how to properly do it at the moment. I know it's doable, because I had to write a database migration system into one of my application a few years ago that was using SQLAlchemy and Alembic, so it's clearly doable.

Exirel · 2022-01-15T18:40:23Z

What's left is:

to decide if you want me to move memory classes into their own submodule of sopel.tools in that PR or not

to write the PR's description, that must contain a list of all the breaking changes and things to consider for the upgrade

See PR tools: move memory classes to tools.memories #2237
Done

This is ready for a last round of review.

dgw

Hm, I found a surprising number of things left to say, despite the many previous comments and despite only re-reviewing about half the files (mostly the ones that "Changed since your last review"). Nobody would ever accuse me of being a code-review Yes Man, huh?

sopel/db.py

sopel/irc/__init__.py

sopel/modules/adminchannel.py

test/test_db.py

sopel/tools/__init__.py

sopel/tools/identifiers.py

Exirel · 2022-01-16T10:09:12Z

Alright this is good, the last review is all about a better presentation and attention to details, I really like it. 👍

Exirel · 2022-01-17T17:18:28Z

That should be good now, @dgw you can read my replies and see if I need to take action or if I can rebase & squash away.

dgw

Just one conversation from before left unresolved—but I started some new ones. Hopefully they won't make you 🙁 too much, since it's about fixing details & consistency in the new changes.

sopel/irc/__init__.py

sopel/modules/adminchannel.py

test/test_db.py

sopel/tools/identifiers.py

Exirel · 2022-01-20T08:19:15Z

And once again! But this time, no new docstrings. 😁

dgw · 2022-01-20T17:48:21Z

I looked, all good. Squash what you're going to squash when ready.

dgw

I'm going to do what I should have done earlier and actually approve this.

You can squash the fixup into the commit SHA I gave and trust me (and CI) that everything will work out post-merge, or rebase on master if you like.

And fix the default casemapping to follow the RFC it is supposed to implement (RFC 1459): only A-Z letters should be lowercased. The lowercase function is now part of a set of public functions, each implement a different "lower" rule: * ascii: implement the "ascii" CASEMAPPING, i.e. A-Z only * rfc1459: implement the "rfc1459" CASEMAPPING, i.e. A-Z + []\~ * rfc1459-strict: implement the same RFC but correctly, i.e. no replacement for ~ (it shouldn't be) The `Identifier` class now use the rfc1459 function, instead of using str.lower. In order to propagate this change correctly, I updated `sopel.db` as well, because str.lower() is not equivalent to Identifier.lower() anymore. Note that this doesn't support unicode Identifier, which would use str.lower directly; the "PRECIS" version of the IRC v3 spec is not ready yet, so it is not implemented for the moment. Also, it would requires to manage not only CASEMAPPING, but also UTF8MAPPING, and that's for another day/feature! See also https://modern.ircdocs.horse/index.html#casemapping-parameter and any links from that document. Co-authored-by: dgw <dgw@technobabbl.es>

Instead of instantiating `tools.Identifier` directly, built-in plugins are now using `bot.make_identifier(nick)` to get an Identifier for nicks and channel. This method comes from `sopel.irc.AbstractBot`, and can be used as an Identifier factory: take a string as input, return an Identifier. In the future, this factory will be able to take advantage of the configuration and the ISUPPORT to select the appropriate CASEMAPPING function. The next steps are: * to use that factory outside of plugins (i.e. triggers and targets) * to implement a different CASEMAPPING depending on context * add documentation about "how to get an Identifier"

Instead of using `tools.Identifier(nick)`, the PreTrigger class now uses a factory to instantiate an Identifier. As a result, the bot (irc.AbstractBot) provides such factory. The Test Factories have been updated to also use the bot's factory when creating test Trigger & PreTrigger. While doing so, type-hint has been added to said factories.

As per PreTrigger, Channel no longuer instantiate Identifier itself: it relies on an Identifier factory to do so. When creating a Channel in coretasks, it provides bot.make_identifier as a factory. Docstrings have been updated, and type-hint have been added for the occasion. Co-authored-by: dgw <dgw@technobabbl.es>

As per PreTrigger and Channel, SopelIdentifierMemory also uses an identifier factory. Sopel and the built-in plugins have been updated accordingly.

The core configuration "nick" option was an Identifier: it was convenient to always have an Identifier for the configured nick. However, it doesn't make a lot of sense: the nick is used in various place as it is (no lowercase), and where it needs to be an Identifier it is always to compare to to another one. As a result: * type(core.nick) is now str * when comparing the config value to an identifier, use bot.make_identifier

And Sopel provides its `make_identifier` method as a factory.

ISUPPORT provide a "CASEMAPPING" parameter, that can tell which casemapping function to use: * ascii: lower A-Z only * rfc1459: ascii + map some special char * rfc1459-strict: ascii + map some special char strictly (no mapping for ~) Whenever the bot received the ISUPPORT parameter, it automatically rebuild its nick according to that parameter. Note: this doesn't implement the UTF8MAPPING parameter; future PR Co-authored-by: dgw <dgw@technobabbl.es>

For type hint, I followed the same type hint provided by SQLAlchemy for connection and session (i.e. return type is an implicit "Any"). Then I switched back the get nick/channel value to accepting an str instead of only an Identifier, since we can always use the Identifier factory now. Co-authored-by: dgw <dgw@technobabbl.es>

Co-authored-by: dgw <dgw@technobabbl.es>

dgw added Core/IRC Protocol Handling Feature labels Dec 30, 2021

Exirel linked an issue Dec 30, 2021 that may be closed by this pull request

Sopel's casefolding is hard-coded #2161

Closed

dgw reviewed Dec 31, 2021

View reviewed changes

Exirel mentioned this pull request Jan 8, 2022

db, test: do not create nick ID by default #2234

Merged

4 tasks

Exirel force-pushed the identifier-casemapping branch from 5705f24 to 02d7044 Compare January 9, 2022 22:46

Exirel added this to the 8.0.0 milestone Jan 10, 2022

Exirel marked this pull request as ready for review January 10, 2022 11:54

Exirel mentioned this pull request Jan 15, 2022

irc: implement CHANTYPES parameter for ISUPPORT #2236

Merged

4 tasks

dgw reviewed Jan 16, 2022

View reviewed changes

dgw added the Breaking Change Stuff that probably should be mentioned in a migration guide label Jan 17, 2022

dgw reviewed Jan 18, 2022

View reviewed changes

Exirel added a commit to Exirel/sopel that referenced this pull request Jan 20, 2022

[TO DROP] Commits from sopel-irc#2231

bc68707

Exirel force-pushed the identifier-casemapping branch from 39541fb to 151a9bf Compare January 20, 2022 17:56

dgw approved these changes Jan 20, 2022

View reviewed changes

Exirel and others added 9 commits January 20, 2022 22:13

docs: update docstrings for tools.identifiers

8adb64f

bot, modules, tools: SopelIdentifierMemory uses an Identifier factory

af1cd92

As per PreTrigger and Channel, SopelIdentifierMemory also uses an identifier factory. Sopel and the built-in plugins have been updated accordingly.

db: SopelDB uses an Identifier factory

01009bb

And Sopel provides its `make_identifier` method as a factory.

identifiers: provide a casemapping function

686f6b2

Exirel and others added 8 commits January 20, 2022 22:13

irc: code simplification for CASEMAPPING

15954a8

tools.identifiers: documentation of casemapping

966a9ab

tools: update SopelIdentifierMemory docstring with the factory

4385ad8

tools.identifiers: tweak docstrings

02ab849

Co-authored-by: dgw <dgw@technobabbl.es>

tools.target: rename nick_id to identifier to avoid confusion

4f478d2

test: improve get_nick_id tests

5f605ea

Exirel force-pushed the identifier-casemapping branch from 4ce843d to 5f605ea Compare January 20, 2022 21:15

dgw merged commit aecc6db into sopel-irc:master Jan 21, 2022

Exirel deleted the identifier-casemapping branch February 23, 2022 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

irc: implement CASEMAPPING parameter for ISUPPORT #2231

irc: implement CASEMAPPING parameter for ISUPPORT #2231

Exirel commented Dec 30, 2021 •

edited by dgw

Loading

dgw left a comment

Exirel commented Jan 9, 2022

Exirel commented Jan 15, 2022

dgw left a comment

Exirel commented Jan 16, 2022

Exirel commented Jan 17, 2022

dgw left a comment

Exirel commented Jan 20, 2022

dgw commented Jan 20, 2022

dgw left a comment

irc: implement CASEMAPPING parameter for ISUPPORT #2231

irc: implement CASEMAPPING parameter for ISUPPORT #2231

Conversation

Exirel commented Dec 30, 2021 • edited by dgw Loading

Description

Non-breaking changes

Breaking changes

Going further

Checklist

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Jan 9, 2022

Exirel commented Jan 15, 2022

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Jan 16, 2022

Exirel commented Jan 17, 2022

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Jan 20, 2022

dgw commented Jan 20, 2022

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Dec 30, 2021 •

edited by dgw

Loading