Remodeling Structures #17
Conversation
| class QueryData: | ||
| """Basic data structure constituting a Dune Analytics Query.""" | ||
|
|
||
| name: str | ||
| query_id: int | ||
| params: Optional[list[QueryParameter]] = None | ||
|
|
||
|
|
||
| class BaseQueryMonitor(ABC): | ||
| class QueryBase(ABC): |
There was a problem hiding this comment.
Here we split the query data into its own class and the base is now a true abstract class. Furthermore, run_loop method has been moved out of this class an into a new class called QueryRunner which has all the third party communication fields like dune and slack client (i.e. those which are required to run query and alert).
| """ | ||
| Elementary implementation of QueryBase that alerts when | ||
| number of results returned is > `threshold` | ||
| """ |
There was a problem hiding this comment.
This was previously the abstract base class. We moved the default implementations out of there so not to accidentally forget to implement methods and fallback on defaults when unintended.
| return self.fixed_params + self.left_bound.as_query_parameters() | ||
| return (self.query.params or []) + self.left_bound.as_query_parameters() |
There was a problem hiding this comment.
This change will be apparent in a few places because python type checking does not allow you to set mutable defaults. This means that the QueryData.params class has to use an Optional[list] defaulting to None instead of a list defaulting to []. The parameters methods expect a list returned and one cannot concatenate None with a list type... this is how we roll.
| """ | ||
| Query Runner takes any implementation of QueryBase, a dune connection and slack client. | ||
| It is responsible for refreshing the query, fetching results, | ||
| passing results onto the query and alerting when necessary. | ||
| """ |
There was a problem hiding this comment.
The heart of the change is here. We introduce QueryRunner. This separates and encapsulates the query details into QueryBase and the third party communication logic for executing and alerting. This makes the unit testing cleaner and should also make the project more maintainable.
| if "window" in cfg: | ||
| # Windowed Query | ||
| window = TimeWindow.from_cfg(cfg["window"]) | ||
| return WindowedQueryMonitor(name, query_id, window, params, threshold) |
There was a problem hiding this comment.
I really like the passing of QueryData object here instead of the many arguments. Another cool quote from Clean Code:
The ideal number of arguments for a function is
zero (niladic). Next comes one (monadic), followed
closely by two (dyadic). Three arguments (triadic)
should be avoided where possible. More than three
(polyadic) requires very special justification—and
then shouldn’t be used anyway.
A more general approach to #15 that is intended to be more long term maintainable.
We separate QueryMonitor structure into three.
QueryDataA data class containing the relevant fields that constitute a Dune queryQueryBase- an abstract class with hollow methods needed to report on the query data and results. Specifically here we introducealert_messageso that the class itself knows how to alert based on its own attributes and result set.QueryRunner- a class containing an implementation of QueryBase, dune connection and slack client. It will be resposible for executing the dune query, passing the results on to the query base implementation for alert message and then posting in slack based on the alert message response.Inline comments on almost every file to make the review easier.
Test Plan
Unit Tests & other CI (type checks linting etc...) There were no logical changes to this project and it was already extensively tested. The only tests that changed were those comparing alert messages since an enum is now included with the alert message.
Note furthermore that I have run all our queries through this code to ensure they still function as expected (this involved commenting out the
slack.post_messagepart. Could probably also implement adry-runfunctionality if we wanted.