Re: [PATCH slidge v2] Implement basic support for WhatsApp

Message ID
DKIM signature
Download raw message
Thanks a lot Alex! I cannot emphasize enough how much I am
 happy that you are contributing to slidge.
> it's unclear what happens when multiple linked devices share a
> contact; how do we choose which account to send from?
Supposedly, contacts are bound to a unique session instance, and
 thus are never shared between different sessions. They are discriminated
 by the stanzas "from" attribute and dispatched to the appropriate session (see `BaseGateway.__register_handlers()`). I might be missing something here; 
have you witnessed some sort of leaking between different sessions?

> danger of hitting rate-limits
Yes, this is something that should definitely be improved. On other plugins 
I do a full roster sync/avatar fetch on every startup but this is not great
 (some libs actually implement caching stuff though). Some persistent store
 for avatar data is needed…

A dirty workaround could be to re-fetch avatars only on message exchange. 
Something like `Contact.avatar_fetched = False` on `__init__` and then wrap
 `Contact.send_text()` and `Session.send_text()` with a `if not c.avatar_fetched: 
c.fetch_avatar()`. But really, we need a way to cache contacts' avatars in core.

> Media messages
I think the Pillow python lib (already a "core" dependency, because XMPP avatars
 need to be PNG) can be used to convert to JPEG when needed.
We need a XEP for XMPP resources to advertise which media formats they accept ^^.
In the meantime, as discussed in the MUC, optionally converting media on the server
 seems sensible, or sending a download URL as fallback is fine. Let it degrade UX for 
whatsapp client users, so that users flee!
> Search and other ad-hoc and chat commands
Since JIDs map to phone numbers, I wonder if a search is really needed here. 
Anyway, many plugins in their current state have no custom commands at all, so this 
is not a problem at all.
> Local development is unavoidably more complicated
You're bridging go and python, perfectly in line with the bridging DNA of slidge. ;)

It seems to work fine EXCEPT on initial launch, because the "generated" dir is empty 
and only modifications in .go files trigger a build. The workaround right now is to just
 touch a .go       file and wait, but I think a cleaner fix is to have an initial build on startup
 if the generated dir is empty or inexistent. (in the watcher.py script maybe?).

Also probably we should rename the ./confs dir to ./dev and put this watcher.py script
 in there to keep things a little more tidy. (later)

> Nevertheless, the plugin is in a good enough state, and has been well-tested
> throughout development, to be in a mergeable state.
I don't doubt it, some other plugins really look less polished than this one :)

For some reason, there is a conflict in gitignore that prevents me from merging your 
patch as it is. Probably need a rebasing first. Anyway, you have push rights now, you'll do it.

And now, for the small code review:

> Gateway.shutdown()

I guess I need to add a Basession.shutdown() method, so that you don't have to 
repeat the content of BaseGateway.shutdown(). I also need that for the skype plugin
, it's going to be DRYer this way. This can be done later.

> attachment_caption for carbons

I was wondering why this variable was not used and then realized carbon_send() 
does no support it.

I'm currently factoring a lot of what's in `LegacyContact` as mixins (so they can be
 reused for MUC participants...), and I really need to make these carbon methods
 homogeneous to their "normal" counterparts. Not sure yet about the right design
 to do this yet, but getting there.

> import asyncio in make_sync()

Why not import at the beginning of the file? Any reason for this?

Other points:

- It would be better to have a separate commit for the typo fix
- Use isort to organize imports. My pre-commit hook uses `isort --profile black 
slidge; black slidge`. I love these auto-formatter tools. Their defaults are fine.
\ They keep diffs small between commits. I love them.
- I cannot review the go parts and I trust your expertise 100% on these .go files.
- There are some mypy errors because of the legacy message types. I am reworking
 this a bit. If you merge to master *before* I finish this rework, just remove the message_id
 type hints.
- Also, we need the go parts to be built on CI so that mypy can at least type check 
the __init__.py file. Mypy is not happy with the generated files so probably need to add exclude="slidge.plugins.whatsapp.generated/*" in the relevant pyproject.toml section.
- A few tests would be nice, but not all plugins have tests, so it's not a hard requirement at 
all (unfortunately). Just checking if the module is actually importable would already
 something, which could be done by something like:
import slidge.plugins.whatsapp as plugin
class TestTelegramBase(SlidgeTest):
    plugin = plugin  # need to use SlidgeTest to not break other tests because of the SubclassableOnce metaclass thing.

All in all, it would be nice to check and/or fix CI-related stuff before merge, 
but we can also merge now and fix later. Your call!

-- Nicoco
Reply to thread Export thread (mbox)