Refactor Part I: Deprecate & Rename Old Routes

[bh2smith]

the functions touched here are:

• refresh --> run_query
• refresh_csv --> run_query_csv
• refresh_into_data_frame --> run_query_dataframe
• get_status --> get_execution_status
• execute --> execute_query
• get_result --> get_execution_results

Maybe a couple others I forgot.

We had to temporarily add a pylint: ignore too-many-public-methods but this will be lifted when we split the client into

DuneExecutionsAPI, DuneQueryAPI, DuneUploadAP - See #72

cc @TheEdgeOfRage & @diegoximenes for additional review.

[msf]

thanks for this, this is in the right direction, I didn't aprove yet because I've found myself with some questions that I left in the PR.

[msf]

deprecate and replace w/ get_query_result() ?

[bh2smith]

This is different than get_query_result. Get latest, fetches the latest results buy query ID (as opposed to execution/job ID).

I suppose we could do as you suggest, but I personally find the two to be rather different.

cf:

https://dune.com/docs/api/api-reference/latest_results/

[msf]

so I don't forget, I need to review this again, but we should have a get_query_result() that uses a Query and simply returns the latest available query result (using the API that exists for that).
This method has the clear difference that it doesnt use up any execution credits, which is important.

[msf]

the other way around, make refresh_csv() use this function instead.

also, what do you think about changing the ordering of the methods in the class?
shall we move:

• old deprecated functions go AFTER the new official ones
• internal helper methods or functions go to the end of the file (or to a separate helper class altogether?)

[bh2smith]

the other way around, make refresh_csv() use this function instead.

Yes, this was just overlooked, will fix.

• I was saving the reordering for the larger refactor (when we split the class into multiple subclasses). This is already proposed in PR Refactor Part II: Split DuneClient Class #72 (where the deprecated functions have been moved to the bottom of their respective new locations).

• Also, internal helper methods have been moved (in Refactor Part II: Split DuneClient Class #72) to a class called ExtendedAPI (naming can easily be changed upon request).

[msf]

Now, looking at the code and having refreshed my memory:

question for you:

• if we have a QueryBase or Query object, shouldn't we have the methods on that class:
• query.execute()
• query.result()
• etc.. ?

with functions like dune.execute_query() it feels more idiomatic this way:

res = dune.execute_query(query_id=1234)

instead of:

res = dune.execute_query(Query(1234))

for me, the two idiomatic options would be:

• dune.execute_query(1234) # or query_id=1234
• dune.Query(1234).execute()

Right now, I see bits of OO in here, but inconsistently:

• Executions are simply job_id
• Queries are objects, with a query_id inside (it should be Query.id, right?)
  • but we don't have methods on Queries..
  • if we did, it would add structure to expand the library for varying types of APIs and do a cleaner split between concepts.

this is just food for thought, the goal here is to improve the code for readability and dev experience in a simple, pragmatic way.

[bh2smith]

I definitely agree some things like this could be approached differently. In all of the ways suggested above. These things have been on my mind for quite some time. I believe the reason we went for query_id is that some of these low level methods are never actually used by the end user and none of the additional query attributes are used. Most consumers of the client are usually calling refresh.

Maybe it can be made compatible with multiple approaches (i.e. introduce some method overrides where users can pass Query object or query_id). I would have to think a bit further if we can extend the functionality to the Query object itself in a backwards compatible way, but I would be wiling to give it a try.

[bh2smith]

I've found myself with some questions that I left in the PR.

I hope some of the questions are now answered (or in some cases at least postponed to the next PR).