-
-
Notifications
You must be signed in to change notification settings - Fork 261
Fix internal IntegrationTests #132
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
@@ -46,14 +46,13 @@ internal class ParseCommand : HttpRequest { | |||
IList<KeyValuePair<string, string>> headers = null, | |||
Stream stream = null, | |||
string contentType = null) { | |||
Uri = new Uri(ParseClient.HostName, relativeUri); | |||
Uri = new Uri(ParseClient.HostName + relativeUri); |
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.
Personally, I dislike this idea of doing it this way, because this requires that either:
- HostName ends with a
/
- or relativeUri begins with a
/
But never both. It works, I'm just afraid of something weird happening here as we move toward 'HostName' being more public.
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 works if both have /
. I've tested this. But if none have /
, it breaks.
The only issue I have with doing new Uri(ParseClient.HostName, relativeUri);
is that
new Uri("https://api.parse.com/1/", "gogo");
translates into
https://api.parse.com/gogo
and not
https://api.parse.com/1/gogo
I'm open to better solution.
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.
I swore the issue was that
new Uri(new Uri("https://api.parse.com/1/"), "/gogo")
resulted in
https://api.parse.com/gogo
Unless I'm reading the Docs wrong...
As long as all of our relativeUris don't have leading slashes (which I thought I got rid of in #77), we should keep the path part of the HostName
.
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.
That makes sense! Let me double-check and update this PR
7e6697f
to
6fcc341
Compare
Fix internal IntegrationTests
Our internal integration tests was broken. This will fix it.
For the case of
HttpClient
, we can't doOnSuccess
. If you take a look inside the code, we do special handling on error.