Refactor: Filters were messy with duplication #58
Conversation
The management of query filters on API requests was coded inside other functions, hard to read and difficult to read. Solution: Rely on a `MessageQueryFilter` object that manages the filters. A new `MessageQuery` interface allows an easier handling of pagination, including iterating transparently iterating over messages with `async for message in MessageQuery(query_filter=filter)`. The interface of `AlephClient.get_messages` has been kept unchanged for now for compatibility reasons.
src/aleph/sdk/client.py
Outdated
| http_client_session=self.http_session, | ||
| ignore_invalid_messages=ignore_invalid_messages, | ||
| invalid_messages_log_level=invalid_messages_log_level, | ||
| ).fetch(page=page, pagination=pagination) |
There was a problem hiding this comment.
Seeing this, I wonder if we should ditch the .fetch(page=page, pagination=pagination) call to allow users to decide whether to fetch a certain page or to iterate through all the messages?
Currently I cannot see how a user would actually access the iterator of MessageQuery.
There was a problem hiding this comment.
The user can always initiate a MessageQuery instance on his own and use it directly. That could even be more elegant, but I am trying to maintain API compatibility.
There was a problem hiding this comment.
Regarding this, I cannot help but feel reminded of the way I implemented similar functionality on the Active Record SDK, see: https://github.com/aleph-im/active-record-sdk/blob/main/src/aars/utils.py
Here, we have IndexQuery which prepares the parameters like MessageQueryFilter does.
The AARS client class handles the actual requests, which is here being handled by MessageQuery.
Then PageableRequest & PageableResponse give all the pagination & iteration over the response of the executed MessageQuery.
I got to admit though, that your approach looks more elegant than what I did in the Active Record SDK, but some of this uglyness resulted in the fact that:
- I am using an ORM and all the functionality needs to be attached to the base class of
Records(which actually represent POSTs). - Some responses are async generators, while some are sync, as AARS does not have a cache capable of async iteration/pagination (while my DomainNode implementation in MessageCache and LightNode #59 does).
I think there is some significant overlap between this PR, the PRs #54 and #59 and the Active Record SDK. Given the right preparation and time for thought, we could build a more coherent and powerful SDK for handling messages as well as POSTs.
One more thing about POSTs: Given the interesting behavior that is possible by amending these messages, I think that they would benefit from the ORM approach chosen in AARS, as it would give users very simple syntax to update/amend POSTs as well as retrieving past revisions of them. This is a paradigm not often seen in DLTs and should be highlighted and easier to work with, IMHO.
|
@MHHukiewitz is this PR deprecated by #65 ? |
|
@hoh yes, it is. I am making an issue out of my last comment, so we can close this |
The management of query filters on API requests was coded inside other functions, hard to read and difficult to read.
Solution: Rely on a
MessageQueryFilterobject that manages the filters.A new
MessageQueryinterface allows an easier handling of pagination, including iterating transparently iterating over messages withasync for message in MessageQuery(query_filter=filter).The interface of
AlephClient.get_messageshas been kept unchanged for now for compatibility reasons.Inspired by #54 from @MHHukiewitz