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.

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: Simplify and improve get_messages; use mocked responses #81

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

MHHukiewitz
Copy link
Member

Inspired by https://github.com/aleph-im/aleph-client/pull/129/files:

  • get_message now requires less parameters and raises ForgottenMessageError if message existed, but was forgotten
    • Uses the /api/v0/messages/{item_hash} endpoint to do so -> should potentially lead to performance improvements
  • The tests in test_asynchronous_get.py are now fully mocked
  • Refactored fixtures from test_asynchronous_get.py and test_asynchronous.py to be included in conftest.py

Solution: Mock responses; refactor to reduce duplication
…en forgotten

Solution: get_messages now calls .../messages/{item_hash} endpoint and raises ForgottenMessageException
@github-actions github-actions bot added the RED This PR is complex and may require more time to review. label Nov 21, 2023
Copy link

The changes in this PR can be considered as having moderate risks due to the following reasons:

  • The addition of a new exception class ForgottenMessageError which extends the base QueryError.
  • Modifications in several functions like get_message, get_messages, and get_posts that involve changes in how they interact with the API, such as using async context managers and handling different response statuses.
  • Addition of new test cases for testing the retrieval of forgotten messages which may impact the system's behavior when dealing with forgotten messages.

The PR also includes some refactoring and documentation updates, but these changes are relatively minor and do not significantly increase the complexity of the review.

In summary, this PR requires a deeper understanding of the project architecture and potential risks involved in handling forgotten messages and different API response statuses. Therefore, it is categorized as 'RED'.

@BjrInt BjrInt self-requested a review November 21, 2023 14:10
@MHHukiewitz MHHukiewitz merged commit 59e22b5 into main Nov 22, 2023
12 checks passed
@MHHukiewitz MHHukiewitz deleted the mhh-forgotten-message-exception branch November 22, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RED This PR is complex and may require more time to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants