Create dataset #8
Conversation
|
Thanks for your PR. I haven't looked over this super thoroughly yet, but my initial feedback is this: This is amazing! Can you add tests? |
|
Definitely. Just ran out of time this morning :P here's what I'm planning to add so far: https://github.com/timwis/sodapy/issues |
|
Rad! I'm glad someone else wants to take this up, because I'm rather busy these days. Can you also add an issue about updating the readme to describe this new endpoint's usage? I should have more time this weekend to review the code and provide more constructive feedback. |
|
Sounds great. I'll try to work on the other items in the meantime |
|
So I've added a test for |
| @@ -7,7 +7,7 @@ | |||
| import json | |||
|
|
|||
|
|
|||
| PREFIX = "mock" | |||
| PREFIX = "http://" | |||
There was a problem hiding this comment.
The requests library drops querystring parameters when the protocol is not http/https. (details)
| @@ -62,7 +62,7 @@ def __init__(self, domain, app_token, username=None, password=None, | |||
| session_adapter["adapter"]) | |||
| self.uri_prefix = session_adapter["prefix"] | |||
| else: | |||
| self.uri_prefix = "https" | |||
| self.uri_prefix = "https://" | |||
There was a problem hiding this comment.
session.mount() works when passed mock but not when https - for https to work, it needs to be passed https://. I'm not sure why this is, but the documentation suggests this approach fits fine. It required a few changes elsewhere, removing :// from a few string constructors. The alternative to doing that is to inject a :// into the session.mount() call, but I chose not to do that in case, in theory, you wanted to use a longer prefix like http://google.com (which would work with this approach)
|
Okay @xmunoz, this pull request is ready to merge on my end! I've added a good number of comments and explanations alongside the changes. Let me know what you think. |
| } | ||
| resource = resource.rsplit("/", 1)[-1] # just get the dataset id | ||
|
|
||
| return self._perform_request("put", "/api/views/" + resource, params=params) |
There was a problem hiding this comment.
Yea, you're right. Doing stuff like this is silly. I like the idea of only requiring the user to pass the 4x4 code + content-type extension and then building the URLs in the calls depending on which version of the API we are hitting. That's kind of asking a lot from you though, so I'll merge this first and do the cleanup in a separate commit.
|
Thanks for all of your work on this! This is really impressive for someone who is just beginning with Python. My comments are minor: Can you delete this at the end of the Also, don't worry about updating the other It's late in Berlin. If you can take care of those two little things, I'll merge in the morning. |
|
Thanks for the encouragement! I've implemented those changes. Let me know when you get a chance to update it in pypi as I intend to use it in this project. Danke und guten nacht! |
Not ready to merge yet. This is a work in progress. Just opening the pull request to encourage discussion around the implementation.(Update: Ready to merge)I'm still new to python and would love feedback and suggestions. I figure there's also room to discuss things like
columns, and the fact that creating a dataset is actually 3 HTTP requests (create, set public, publish). Here's what I have so far.Edit: Another thing worth discussing is the fact that this uses the new API, so
/resource/asdf-asdf.jsonis not the correct path (it's/api/views/asdf-asdf.json). I'd make the argument for abstracting the resource in the library and only looking for the 4x4 dataset ID. This way it would be very easy to switch from the old API to the new one, even for reading/GETs. I imagine in order to make this a non-breaking change, we'd have to allow the/resource/xxxx.jsonusage and just trim it off the string as I've done in this pull request.