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: DuneInterface.refresh return type #26

Merged
merged 1 commit into from
Oct 27, 2022
Merged

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Oct 27, 2022

In order for users to cache results, they need access to the JOB ID of their execution. The refresh method now returns more enriched data giving these advanced options to the user.

We lost the convenience of not having to check for an optional field, but introduced a similar convenience in the ResultsResponse (i.e. the new method get_rows which performs the necessary assertion check) and always returns a list of dune records.

Bonus feature is the Query__hash__ implementation which will allow people to cache results of different executions of the same query with different parameters.

Closes #25 and closes #27

@bh2smith bh2smith requested a review from a team October 27, 2022 10:55
@bh2smith bh2smith changed the title Refactor: DuneInterface refresh return type Refactor: DuneInterface.refresh return type Oct 27, 2022
Comment on lines +37 to +42
def __hash__(self) -> int:
"""
This contains the query ID and the values of relevant parameters.
Thus, it is unique for caching purposes
"""
return self.url().__hash__()

Choose a reason for hiding this comment

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

Like this idea for hashing!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this did not work out as I had expected:

#32

query_id: int
name: Optional[str] = "unnamed"

Choose a reason for hiding this comment

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

Also liked you've added the name. Going to be using this in the excel export!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The name was always here, but this just makes it optional with a default value so that it's less annoying when you don't want to use it.

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!

@bh2smith
Copy link
Collaborator Author

This is an interface breaking change so we will have to bump the version to 0.1.0

@bh2smith bh2smith merged commit 55d5f92 into main Oct 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 27, 2022
@bh2smith bh2smith deleted the refactor-interface branch October 31, 2022 12:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change refresh to return full response Make Query.name optional with default
2 participants