-
Notifications
You must be signed in to change notification settings - Fork 16
Refactor: Migrate message commands to aleph-sdk-python #150 #163
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.
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
Refactor: Migrate message commands to aleph-sdk-python #150 #163
Conversation
The Aleph API accepts either a float or a datetime object, which caused confusion for mypy. Solution: We always convert 'date' to a datetime object to resolve this issue.
src/aleph_client/commands/message.py
Outdated
channels_s: Optional[list[str]] = None | ||
chains_s: Optional[list[str]] = None | ||
|
||
content_types_s = content_types.split(",") if content_types else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why _s
? These variables are not strings. Maybe use something like parsed_content_keys
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_s = _splited in my head
src/aleph_client/commands/message.py
Outdated
@@ -25,6 +25,17 @@ | |||
app = typer.Typer() | |||
|
|||
|
|||
def str_to_datetime(date: Optional[str]) -> Optional[datetime]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a docstring to explain that this function can take either a timestamp or an ISO format datetime as parameter.
tests/integration/itest_posts.py
Outdated
timeout=5, | ||
hashes=[post_message.item_hash], | ||
api_server=receiver_node, | ||
data = {'content': 'test'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dictionary appears to be unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests/integration/itest_posts.py
Outdated
async with AuthenticatedAlephClient( | ||
account=fixture_account, api_server=sdk_settings.API_HOST | ||
) as client: | ||
result, status = await client.create_post( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename result to message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, now that the SDK returns a status, you can assert that the operation worked with assert status == ...
.
tests/integration/itest_posts.py
Outdated
response = await try_until( | ||
get_message, | ||
lambda r: r is not None and r.content is not None, | ||
timeout=50, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 50? Revert to 5, that's too long. Note that you can also specify sync=True
in client.create_post
to make sure that the message was processed on the API node.
From : #150
Refactor: