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

Preemptive Refactor #15

Closed
wants to merge 3 commits into from
Closed

Preemptive Refactor #15

wants to merge 3 commits into from

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Aug 11, 2022

In preparation for a new type of Query Monitor (Counter type) we had to generalize some parts of the code to avoid duplication in the next extension class. Specifically we move slack client and dune connection into the query monitor class so that run_loop can be split into two parts (for more reusable components).

Unfortunately this mean we temporarily initialize the class with dummy versions of these classes only to set them after instantiation (this is a hack so that unit tests don't require env vars be set).

The plan is to move into something more general (although I have to do some design sketch before).

NOTE the main goal of this is to make #16 easier to read. These will be followed up with a more general refactor aimed at cleaning up the abstraction patterns and reducing redundancy.

@bh2smith bh2smith requested a review from a team August 11, 2022 08:47
Copy link

@gentrexha gentrexha left a comment

Choose a reason for hiding this comment

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

LGTM!

Like the generalization with the slack_client.

@bh2smith bh2smith mentioned this pull request Aug 25, 2022
@bh2smith
Copy link
Contributor Author

Closed in favour of #17

@bh2smith bh2smith closed this Aug 29, 2022
@bh2smith bh2smith deleted the preemptive-refactor branch September 5, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants