-
Notifications
You must be signed in to change notification settings - Fork 11
Create class to handle absolute urls #81
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
Conversation
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.
In the original issue, it was about improving request_rest
, while this stands up a new method. Should this be baked into request_rest
so consumers benefit just from upgrading, or is this potentially breaking and should have people migrate to this new method?
] | ||
|
||
def get_url(self): | ||
return "" # Not used since we expect full URLs |
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.
need to define this? the base of this func throws an error, prob can keep if we don't want people to use this
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 intentionally defined as an empty string, otherwise request_rest tries to append whatever URL is passed in to whatever is returned in get_url
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 that's the case then yeah def, but doesn't this function get inherited from here? I'm not a python guy so idk how inheritance works here
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 does get inherited from there but we are overriding it here by returning an empty string
if should_raise: | ||
with pytest.raises( | ||
ValueError, | ||
match="URL must start with one of: https://app.useanvil.com", |
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.
ideally this uses VALID_HOSTS
somehow
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 should_raise: | ||
with pytest.raises( | ||
ValueError, | ||
match="URL must start with one of: https://app.useanvil.com", |
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.
ideally this uses VALID_HOSTS
somehow
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.
👍
@@ -117,6 +117,10 @@ def request_rest(self, options: Optional[dict] = None): | |||
api = RestRequest(self.client, options=options) | |||
return api | |||
|
|||
def request_fully_qualified(self, options: Optional[dict] = None): |
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.
idt this is actually used anywhere - looks like the func above is used here. a couple other placess too, probably should use this new version
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.
Yeah, request_rest also doesnt seem to be called anywhere. I guess someone could use these if they wanted to when making other requests to Anvil
Closes #79
Description
This PR addresses Issue #79 where passing a fully-qualified URL to request_rest.get() or request_rest.post() would incorrectly prepend the base URL, resulting in an invalid request.
Changes