Trusted (Write) API #27
Conversation
tbapy/main.py
Outdated
| """ | ||
| url = "event/{}/rankings/update".format(self.auth_event_key) | ||
| data = json.dumps(data) | ||
| concat = self.auth_secret + "/api/trusted/v1/" + url + str(data) |
There was a problem hiding this comment.
I don't think you need the str() here, since json.dumps returns a string.
There was a problem hiding this comment.
Fixed in 3875cd9
tbapy/main.py
Outdated
| :param data: Dictionary of breakdowns and rankings. Rankings are a list of dictionaries. | ||
| """ | ||
| url = "event/{}/rankings/update".format(self.auth_event_key) | ||
| data = json.dumps(data) |
There was a problem hiding this comment.
Seems a little unnecessary to use all these variables and only use them once.
There was a problem hiding this comment.
At least given the style we use for the code right now.
There was a problem hiding this comment.
I think I finally figured out how to write it as simply as possible so every function doesn't use variables only once. Check out b5abbdd
tbapy/main.py
Outdated
|
|
||
| :param data: Dictionary of breakdowns and rankings. Rankings are a list of dictionaries. | ||
| """ | ||
| url = "event/{}/rankings/update".format(self.auth_event_key) |
There was a problem hiding this comment.
We use single quotes (') elsewhere in the code. Might want to use that here instead of ". Also, we're using the % notation rather than .format() right now so it would be nice if you conformed to that.
tbapy/main.py
Outdated
|
|
||
| def update_event_info(self, data): | ||
| """ | ||
| Update an event's info on The Blue Alliance. |
There was a problem hiding this comment.
Should this maybe be called just update_event? I don't know, I'm not familiar with the write API's naming.
There was a problem hiding this comment.
All of the functions take the last two tokens in the URL, switch them around, and put "event" in between. I didn't put too much thought into it cause I expected you to want to change them.
tbapy/main.py
Outdated
| self.auth_secret = auth_secret | ||
| self.auth_event_key = auth_event_key | ||
|
|
||
| def update_event_info(self, data): |
There was a problem hiding this comment.
data is kind of a weird name. Also, should we maybe make this use an Event object instead, rather than a JSON dictionary? Either way is fine but using an object might be better. This might actually support both...
There was a problem hiding this comment.
Data is a generic name. Using an Event object isn't impossible but not all of the functions can accept one.
tbapy/main.py
Outdated
| url = "event/{}/match_videos/add".format(self.auth_event_key) | ||
| data = json.dumps(data) | ||
| concat = self.auth_secret + "/api/trusted/v1/" + url + str(data) | ||
| return self._post(url, data, md5(concat.encode("utf-8")).hexdigest()) |
There was a problem hiding this comment.
What's the reason for encoding as utf-8 and putting it into md5 and hexdigesting? I don't doubt there's a good reason but I don't know what it is.
There was a problem hiding this comment.
The basic explanation is that every string in python3 is a Unicode object and the md5 function from hashlib requires an encoded object before hashing. Hexdigest is just the function to return the hashed string.
|
@AndrewLester Check this out. |
tbapy/main.py
Outdated
| :return: Requests Response object. | ||
|
|
||
| """ | ||
| return post(self.WRITE_URL_PRE + url % self.auth_event_key, data=data, headers={'X-TBA-Auth-Id': self.auth_id, 'X-TBA-Auth-Sig': md5((self.auth_secret + '/api/trusted/v1/' + url + data).encode('utf-8')).hexdigest()}) |
There was a problem hiding this comment.
I think you could do json=data instead, which would convert the data to json automatically instead of you having to use json.dumps.
There was a problem hiding this comment.
The only reason I do json.dumps earlier is because the auth-sig needs it to be dumped before hashing.
|
|
||
| :param data: Dictionary of data to update the event with. | ||
| """ | ||
| return self._post('event/%s/info/update', json.dumps(data)) |
There was a problem hiding this comment.
Do you think it would be better to have the user specify the event key inside each function or as an instance variable? (Also, it might be advantageous to change the name of the variable from auth_event_key to event_key anyway. Why "auth"?) Just wondering what you think @NikhilNarayana and @ErikBoesen
There was a problem hiding this comment.
I used auth_event_key because each auth_id and auth_secret are linked to a single event so I believe it makes the most sense to have the event_key set for any instance of the class. The variable name isn't that big of a concern to me, when I put "auth" in the name I was thinking of authorized event key.
There was a problem hiding this comment.
I decided to just change it to event_key and update the doc strings. 4b07015
|
No problem! Just happy to help. |
|
@NikhilNarayana Hey, a fun tip: you can write "Fixes #22" in the description of your pull request (or "Resolves", "Fix", "Closes", or variations thereupon), causing the referenced issue to automatically be closed upon the merging of that PR. Just a tip for the future. |
|
I made a small error when I tried to simplify the code that prevents the correct X-TBA-Auth-Sig header from being generated. The fix is really simple: 1c3d037 Sorry for not testing it beforehand. |
|
Can you make a new pull request to fix this? |
I use this library a lot and wanted to introduce the write API into it so I could quit using my own code. I tried to stick to your code style but I'm sure I made some mistakes. I tested the updates on my TBA dev server to make sure it works in general and no bugs came up. I would also consider adding a decorator on all of the write functions to dump the data and check if the instance has an ID, secret, and event key.
Some extra notes, I didn't change anything in models since I wasn't sure how to properly update it. I set the _post function to return the Response object that requests provides.