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

Feature: get_message_status and overload of get_message to return status #163

Merged
merged 3 commits into from
Sep 3, 2024

Conversation

1yam
Copy link
Collaborator

@1yam 1yam commented Aug 30, 2024

Problem: Currently, there is no method to retrieve the status of a message using its item_hash. This limitation makes it difficult to efficiently check the processing state of specific messages.

Solution: I have added a new method, get_message_status, which allows users to obtain the status of a message by providing the item_hash. I have updated the get_message method to optionally return the status of a message when requested, improving overall flexibility and functionality.

Copy link


Summary and Analysis

The PR introduces significant changes to the http.py file, including:

  1. Imports and Type Annotations: The file now includes numerous new imports and complex type annotations, which could potentially introduce bugs or misunderstandings.
  2. Overloads: The addition of @overload declarations for get_message and get_message_status methods introduces complexity, potentially affecting the readability and maintainability of the code.
  3. Status Handling: The introduction of with_status parameter and handling of status responses adds additional complexity and could affect the overall stability of the function.

These changes are likely to have a high potential for introducing bugs or requiring deep understanding of the project architecture, making them suitable for a 'BLACK' level of review.

@github-actions github-actions bot added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Aug 30, 2024
philogicae
philogicae previously approved these changes Aug 30, 2024
Copy link
Member

@philogicae philogicae left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@hoh hoh left a comment

Choose a reason for hiding this comment

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

No test on this ?

src/aleph/sdk/client/http.py Outdated Show resolved Hide resolved
Copy link
Member

@philogicae philogicae left a comment

Choose a reason for hiding this comment

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

Reviewed, fixed and ready

@philogicae philogicae merged commit c62fbb7 into main Sep 3, 2024
13 checks passed
@philogicae philogicae deleted the 1yam-message-status branch September 3, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLACK This PR has critical implications and must be reviewed by a senior engineer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants