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:
```python
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