Inconsistant contract for 'RoomOccupant' / 'Identifier'

webpigeon commented 3 years ago

In order to let us help you better, please fill out the following fields as best you can:

I am...

[ ] Reporting a bug
[ ] Suggesting a new feature
[ ] Requesting help with running my bot
[ ] Requesting help writing plugins
[X] Here about something else

I am running...

Errbot version: 6.1.7
OS version: Fedora/Ubuntu
Python version: 3.9
Using a virtual environment: yes

Issue description

The contract for an an 'Identifier' needs is poorly defined. The RoomOccupant class ( https://errbot.readthedocs.io/en/latest/_modules/errbot/backends/base.html#RoomOccupant ), only defines one method as being required (room). The description states it should return the full name (which I'm a little confused about, but I'm assuming it wants the room ID).

ErrBot core throws an exception if there isn't a person method present:

Traceback (most recent call last):
  File "/home/webpigeon/Documents/fossgalaxy/projects/errbot/test/../backends/matrix/errmatrix.py", line 296, in on_message
    await self._bot.loop.run_in_executor(None, self._bot.callback_message, msg)
  File "/usr/lib64/python3.9/concurrent/futures/thread.py", line 52, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/home/webpigeon/.local/share/virtualenvs/errbot-CvTTjbXc/lib/python3.9/site-packages/errbot/core.py", line 698, in callback_message
    if self.process_message(msg):
  File "/home/webpigeon/.local/share/virtualenvs/errbot-CvTTjbXc/lib/python3.9/site-packages/errbot/core.py", line 257, in process_message
    raise Exception(
Exception: msg.frm not an Identifier as it misses the "person" property. Class of frm : <class 'errbot.backends.errmatrix.MatrixRoomOccupant'>.

The error message states that identifiers need to have a person property, this is incorrect as the documentation also states (and implements) that rooms are Identifiers, which clearly doesn't have a person property. The identifier also doesn't have an abstract method called person or make any reference to it (but Person does).

We've used errbot for a long time and we're really impressed with what it can do, I'm more than happy to submit a PR with documentation fixes, I just don't know what the requirements are :).

So, my questions are:

What are the requirements for an Identifier and a RoomOccupant?
Should I be passing a RoomOccupant as frm (I was passing a Person but then the chatroom plugin crashes if it's a public room)?
(aside) I've had to override what a private message (used by Admin PM-only commands) because of the way Matrix works, https://git.fossgalaxy.com/irc/errbot/backend-matrix/-/blob/feature-better-async/errmatrix.py#L162 I've overridden BuildMessage and BuildReply, is there anywhere else I need to be careful of ( minus Cards, which are next on my list :) )

nzlosh commented 3 years ago

I didn't design errbot and I agree having an abstract base class that defines nothing does make for a weak API contract. I took a look at other backends and most use multiple inheritance of Person and RoomOccupant, however this probably only makes sense in the context of how the particular backend actually functions.

I did find a couple of exceptions to the multiple inheritance:

RocketChat: https://github.com/cardoso/errbot-rocketchat/blob/59ea43c1d2023525ba32ad4e5ea96b31340357bf/src/rocketchat/backends/rocketchat.py#L185 (only has a Person object, I don't know how a Room is represented for this backend).
BotFramework: https://github.com/vasilcovsky/errbot-backend-botframework/blob/da15011afcd66a83e1a26dba5ab37a1f8b36bcfc/botframework.py#L81 (Looks like it re-implements the Identifier class).

The Identifer class was added with this commit.

Author: Guillaume Binet <gbin@google.com>
Date:   Mon Nov 9 14:42:28 2015 -0500

    Materialized Identifier and MUCIdentifier as ABC

    With type hinting on the API surface, it makes more sense now to
    materialize the Identifiers so users can autocomplete/explore them.

When committed, it had the following method: person, client, nick, aclattr, fullname.

A later commit removed all methods

commit b9ae4d06ed9668694ce5ea54580acd2f0b412c04
Author: Guillaume Binet <gbin@google.com>
Date:   Fri Mar 4 16:10:44 2016 -0800

    big rename, still some failing cases.

The methods removed from Identifier are all part of the Person class.

From the examples I've seen, I'd recommend using multiple inheritance and if that causes crashes in the plugin, try to investigate what underlying assumptions in the code aren't compatible with that backend and try work around it. I hope that helps.

webpigeon commented 3 years ago

Thanks very much, based on that I was able to get my Matrix backend working (it also has reaction support now :)).

errbotio / errbot