Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JSON-RPC python client #3734

Merged
merged 25 commits into from
Dec 4, 2022
Merged

JSON-RPC python client #3734

merged 25 commits into from
Dec 4, 2022

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Nov 5, 2022

Moved here from #3695

@link2xt link2xt mentioned this pull request Nov 5, 2022
Merged
@link2xt link2xt force-pushed the link2xt/async-jsonrpc-client branch 4 times, most recently from eecfab2 to 17f3094 Compare November 8, 2022 21:56
@link2xt link2xt force-pushed the link2xt/async-jsonrpc-client branch 3 times, most recently from 83dc816 to d6b93d0 Compare November 16, 2022 20:11
@link2xt link2xt force-pushed the link2xt/async-jsonrpc-client branch 2 times, most recently from 0d661e8 to 322d765 Compare November 29, 2022 17:12
@link2xt link2xt mentioned this pull request Nov 29, 2022
Merged
@link2xt link2xt marked this pull request as ready for review November 29, 2022 18:45
@link2xt link2xt mentioned this pull request Nov 29, 2022
Closed
@link2xt
Copy link
Collaborator Author

link2xt commented Nov 29, 2022

I think it is sufficiently good for review and merge, we have a nice echo bot that uses only object-oriented API in examples folder and some tests.

@link2xt link2xt force-pushed the link2xt/async-jsonrpc-client branch 2 times, most recently from 0582cca to cc57387 Compare November 29, 2022 23:08
adbenitez
adbenitez reviewed Nov 30, 2022
View reviewed changes
deltachat-rpc-client/examples/echobot.py Outdated
fresh_message_snapshots = await asyncio.gather(*fresh_message_snapshot_tasks)
for snapshot in reversed(fresh_message_snapshots):
if not snapshot.is_info:
await snapshot.chat.send_text(snapshot.text)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this never raise exception? example if bot was removed from group then get message due to email races, then message will never be marked as read and will be stuck failing always with that message

adbenitez
adbenitez reviewed Nov 30, 2022
View reviewed changes
deltachat-rpc-client/examples/echobot.py Outdated

while True:
event = await account.wait_for_event()
if event["type"] == "Info":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to provide an StringEnum EventType in the lib so user can compare with EventType.INFO instead of having to know the exact name of the event types

@adbenitez
Copy link
Member

I created #3790 fixing some bugs and implementing some of my comments here

@missytake
Copy link
Contributor

Are you writing this for deltachat-bot/bot-pages#45 ? Because the python example in this PR is still kind of complicated, and now you have to know async... which is not the kind of simplicity I was aiming for in deltachat-bot/bot-pages#45. It does look pretty cool though and makes sense on its own, I think :) But maybe deltachat-bot/bot-pages#45 should not wait for this PR?

@link2xt link2xt mentioned this pull request Dec 3, 2022
Merged
@link2xt
Copy link
Collaborator Author

link2xt commented Dec 3, 2022

Are you writing this for deltachat-bot/bot-pages#45 ?

No, see the full comment here: deltachat-bot/bot-pages#45 (comment)

@link2xt
Properly terminate Rpc and remove sleep() hack
5a30653
@link2xt
Remove start_rpc_server() and make Rpc a context manager
2ccf398 
Rpc now has a start() method.
This way it is possible to use Rpc from IPython without calling __aenter__()
@link2xt link2xt requested a review from flub December 4, 2022 13:59
hpk42
hpk42 reviewed Dec 4, 2022
View reviewed changes
deltachat-rpc-client/src/deltachat_rpc_client/rpc.py
Comment on lines +68 to +73
def __getattr__(self, attr: str):
async def method(*args, **kwargs) -> Any:
self.id += 1
request_id = self.id

assert not (args and kwargs), "Mixing positional and keyword arguments"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess it ok for now to do this dynamically. But for documentation purposes and for autocompletion it would be much nicer if there are spelled out methods with paramters and docstrings.

hpk42
hpk42 approved these changes Dec 4, 2022
View reviewed changes
Copy link
Contributor

@hpk42 hpk42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicely done. I'd prefer there to be more tests for the Message, Chat and Contact objects than there are now. What is also missing is documentation. But i guess none of this needs to block things right now.

@link2xt link2xt merged commit b4f348c into master Dec 4, 2022
@link2xt link2xt deleted the link2xt/async-jsonrpc-client branch December 4, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adbenitez adbenitez adbenitez approved these changes

@flub flub flub approved these changes

@hpk42 hpk42 hpk42 approved these changes

@Simon-Laux Simon-Laux Awaiting requested review from Simon-Laux

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@link2xt @adbenitez @missytake @hpk42 @flub @Simon-Laux