Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PRO-937: Add
create
andinsert
table endpoints to sdk #114PRO-937: Add
create
andinsert
table endpoints to sdk #114Changes from all commits
3853295
f97dc83
fd95ef8
dd6d30d
6a3a1c5
2f14b9a
3338e36
eaa0fdb
8c7f8e6
6130465
d5fe607
e3f6c98
0c1c14d
7fdbfa5
4e11311
642cc49
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some people might still be depending on this. We might need to keep it for backwards compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was moved here
https://github.com/duneanalytics/dune-client/pull/114/files/dd6d30d78502ea752bd6c04cc1ea7299da2e1347#diff-480e443b4b14d9d3ff5327a749050ff3e17aa60adbdd304fa90b524d89644073R19
Experience for users would not change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: There is no landing page for the Tables API. This url redirects to https://docs.dune.com/api-reference/tables/endpoint/create and there only appears to be one direct link in the docs page (inside a comment on uploadCSV)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you are correct.
Opening a ticket internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great callout! It should probably be an "Overview" or similar like with the other endpoint groups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we probably want to create an enum of the possible types, but not yet!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you happen to have a list of all the possible types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup they're defined here
https://dune-tables-docs.mintlify.app/api-reference/tables/endpoint/create
(Link is in docstring)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
Any
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what
self._post
returns.This was already implemented, but we can change that once we have something returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, should this change now? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to say that
Any
is not a useful return type. What should users expect back when calling thecreate_table
orinsert_table
functions ?But we should already have a response returned from the API, right ?
The fact that
self._post
returnsAny
, might have been a bad design / tech debt. If this is part of the public API, which we cannot change, maybe we can consider returning a more useful type here.If not an easy change, let's keep it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it was probably not the best decision.
This function is kind of unnecessary, which is used to handle
_post
,_get
, and_patch
responsesdune-client/dune_client/api/base.py
Lines 139 to 150 in e3f6c98
Should have just returned the
Response
object.response.json()
returnsAny
, so this will take a bit of work.And as such, I don't think is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take this back, this was not as simple as I thought it would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🖖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry folks, this was my fault☹️ . But these internal private methods return Any and the idea is to convert them to the expected type inside these pubic facing interfaces. So, for example... we have these Object.from_dict() methods that convert Any responses to the expected types:
dune-client/dune_client/api/execution.py
Lines 66 to 72 in b9db190
Here would have been the same thing. Since I am currently implementing these API routes in the Node project, I will submit a corresponding PR for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the related PR: #117