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 Part II: Split DuneClient Class #72

Merged
merged 12 commits into from
Sep 8, 2023
Merged

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Sep 6, 2023

We split the DuneClient class up with an inheritance structure described vaguely as follows:

    Inheritance Hierarchy sketched as follows:

    DuneClient
        |
        |--- QueryAPI(BaseRouter)
        |       - Contains CRUD Operations on Queries
        |
        |--- ExtendedAPI
                | - Contains compositions of execution methods like `run_query` (formerly refresh)
                |
                |--- ExecutionAPI(BaseRouter)
                        - Contains query execution methods.

Note that this is based on #64 (which should be merged first).

Closes #69

cc @TheEdgeOfRage & @diegoximenes for additional review.

@bh2smith bh2smith changed the base branch from main to deprecate-old-routes September 6, 2023 08:11
@bh2smith bh2smith requested a review from msf September 6, 2023 08:14
@bh2smith bh2smith force-pushed the deprecate-old-routes branch from 202f26d to 6bc5f80 Compare September 6, 2023 12:33
@bh2smith
Copy link
Collaborator Author

bh2smith commented Sep 6, 2023

Note that we can circle back and clean this up even more later. This is mostly just an initial proposal for the final structure and servers as a starting point from which we can move things around as we see fit.

@bh2smith bh2smith mentioned this pull request Sep 6, 2023
@bh2smith bh2smith force-pushed the deprecate-old-routes branch from 10b15a0 to c21da35 Compare September 7, 2023 07:31
@bh2smith bh2smith changed the title Split DuneClient class Refactor Part II: Split DuneClient Class Sep 7, 2023
Base automatically changed from deprecate-old-routes to main September 7, 2023 09:56
Copy link
Collaborator

@msf msf left a comment

Choose a reason for hiding this comment

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

Almost a SHIP IP, but see my suggestions and comments, they're tiny and fast to address (mostly about comments and moving functions up/down the file)

dune_client/api/base.py Show resolved Hide resolved
dune_client/api/base.py Show resolved Hide resolved
dune_client/api/base.py Show resolved Hide resolved
dune_client/api/extensions.py Outdated Show resolved Hide resolved
dune_client/api/extensions.py Outdated Show resolved Hide resolved
dune_client/api/extensions.py Show resolved Hide resolved
dune_client/client.py Outdated Show resolved Hide resolved
dune_client/api/extensions.py Outdated Show resolved Hide resolved
dune_client/api/extensions.py Outdated Show resolved Hide resolved
dune_client/client.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Just passed through your comments. Will address all the ones with thumbs up ASAP.

dune_client/api/base.py Show resolved Hide resolved
dune_client/api/extensions.py Show resolved Hide resolved
dune_client/api/extensions.py Outdated Show resolved Hide resolved
@bh2smith bh2smith requested a review from msf September 7, 2023 12:30
Copy link
Collaborator

@RichardKeo RichardKeo left a comment

Choose a reason for hiding this comment

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

Thanks for your work @bh2smith. I only did a quick pass and it looks good to me. I left a few comments, but they are nitpicks for the most part. Feel free to ignore them if you disagree.

dune_client/api/base.py Show resolved Hide resolved
dune_client/api/base.py Show resolved Hide resolved
dune_client/api/base.py Show resolved Hide resolved
dune_client/api/base.py Show resolved Hide resolved
dune_client/api/extensions.py Show resolved Hide resolved
dune_client/api/extensions.py Outdated Show resolved Hide resolved
dune_client/api/extensions.py Show resolved Hide resolved
dune_client/api/query.py Outdated Show resolved Hide resolved
dune_client/api/query.py Outdated Show resolved Hide resolved
dune_client/api/query.py Outdated Show resolved Hide resolved
@bh2smith
Copy link
Collaborator Author

bh2smith commented Sep 7, 2023

Thanks @RichardKeo for your careful review. I will create relevant issues and make suggested changes here accordingly first thing tomorrow!

bh2smith and others added 8 commits September 8, 2023 09:56
Co-authored-by: Miguel Filipe <msf@users.noreply.github.com>
Co-authored-by: Miguel Filipe <msf@users.noreply.github.com>
Co-authored-by: Miguel Filipe <msf@users.noreply.github.com>
Co-authored-by: Miguel Filipe <msf@users.noreply.github.com>
@bh2smith
Copy link
Collaborator Author

bh2smith commented Sep 8, 2023

Okie, I have addressed all the comments and suggestions here (@RichardKeo). Please let me know if this is ready for 🚢 . Everything else will follow much easier from here (as everything is currently based on this branch).

Copy link
Collaborator

@RichardKeo RichardKeo left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

return self.get_execution_results_csv(job_id)

def run_query_dataframe(
self, query: QueryBase, performance: Optional[str] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you still planning to add ping_frequency here?

@bh2smith
Copy link
Collaborator Author

bh2smith commented Sep 8, 2023

In light of the backlog depending on this and all the comments addressed, I will merge this now. We can always double back and make additional changes upon request.

@bh2smith bh2smith merged commit 9314591 into main Sep 8, 2023
4 checks passed
@bh2smith bh2smith deleted the split-client-class branch September 8, 2023 10:33
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.

Break Client into Family of Classes
3 participants