-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add upload_csv route #61
Conversation
0146d70
to
9628db7
Compare
@@ -9,7 +9,6 @@ | |||
from typing import Dict | |||
|
|||
|
|||
# pylint: disable=too-few-public-methods |
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.
This is no longer needed (thought I would sneak it in here).
The ability to overwrite other people's tables under |
""" | ||
response_json = self._post( | ||
route="/table/upload/csv", | ||
params={ |
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.
@dsalv how do we specify the namespace the table should be in?
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.
right now it is not possible to customize the namespace. All publicly visible tables go as dune_upload.{table_name}
. All tables are also duplicated as dune.{username}.dataset_{table_name}
.
The current plan is to keep the personal namespaces and deprecate dune_upload
soon. There is no plan to support customizable namespaces at the moment. This needs to be escalated if we think customizable namespaces are critical
So far, it looks like just commentary on intended functionality. At the moment the implementation and tests are aligned with the intended functionality. If its not blocking, I'd think we can merge as is and adjust later as necessary. Please let me know if there is anything that should be changed here and feel free to approve if all is good. |
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.
lgtm
This PR adds CSV upload functionality to the API client.
Unfortunately, it appears, I have the ability to completely overwrite other users tables... Not sure if this was intended. cc @msf
Tests are failing, because I assumed I would get a False success response when trying to overwrite someone else's table.
Closes #67
Remaining Question: Do we also was to update the async client? If so, it would probably need more work since it has not been kept updated like the other.