Feature/support boot timeout #1
Conversation
heroku3/models/app.py
Outdated
| r = self._h._http_resource( | ||
| method="DELETE", | ||
| resource=("apps", self.id, "limits", "boot_timeout") | ||
| ) | ||
| return r.status_code |
There was a problem hiding this comment.
can these use _get_resource instead of _http_resource and can you please throw together a return value object for this? Just wanna be consistent with the other stuff.
There was a problem hiding this comment.
_get_resource only uses GET AFAICT, so there should probably be a _delete_resource method for consistency, I guess.
I agree with the consistency argument, but I'm not sure in this case which return value object I should create, since DELETEing only returns a status code.
There was a problem hiding this comment.
can u find some other instances of DELETE in the codebase and check out what they do there?
There was a problem hiding this comment.
"""
Removes a collaborator from a project
options = id_or_email
"""
r = self._h._http_resource(method="DELETE", resource=("apps", self.name, "collaborators", id_or_email))
r.raise_for_status()
return r.ok
yeah, it was super easy :/
raise_for_status() seems redundant, as it's already executed by _http_resource, https://github.com/smartpension/heroku3.py/blob/master/heroku3/api.py#L209
but a lot of other functions call it anyway, so...
|
|
||
| obj = r.json() | ||
| assert obj["name"] == "boot_timeout" | ||
| return obj["value"] |
There was a problem hiding this comment.
Re: the return value object, I could probably create a BootTimeout or Limit object and return that for consistency, although it feels a bit over-engineered (we're returning an int after all). I'll try to come up with something.
| raise | ||
|
|
||
| obj = r.json() | ||
| assert obj["name"] == "boot_timeout" |
There was a problem hiding this comment.
This is not done anywhere else as far as I can tell, but I think a sanity check is always better to have than not to have.
Probably it would be clearer to raise a RunTimeError as anything else than that string, as well as any error raised on line 350, would be a violation of the contract by the APIs.
There was a problem hiding this comment.
if we can find any other isntances of raising if response isn't what we expect and can copy their pattern, that would be amazing. One of the benefits of the value object you're gonna add is that it will fail to initialize if the response isn't the right format though so that should help 😂
|
Closing due to branch renaming. |
|
@bclewis1 I forgot why this PR is against |
No description provided.