make API non-throwing #66

tomerd · 2019-07-10T01:26:58Z

motivation: cleaner API for users

changes:

make request initializer optional instead of throwing
http "sugar" methods (get, post, etc) no longer need to be throwing
fix schema logic to be non-case sensitive
made request object version, method and url immutable
made request object scheme and host internal
adjusted redirect ahndler implementation to stricter request immutabllity
adjust and add tests

motivation: cleaner API for users changes: * make request initializer optional instead of throwing * http "sugar" methods (get, post, etc) no longer need to be throwing * fix schema logic to be non-case sensitive * made request object version, method and url immutable * made request object scheme and host internal * adjusted redirect ahndler implementation to stricter request immutabllity * adjust and add tests

Yasumoto · 2019-07-10T03:28:26Z

Sources/AsyncHTTPClient/HTTPHandler.swift

-            guard let scheme = url.scheme else {
-                throw HTTPClientError.emptyScheme
+        public init?(url: URL, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) {
+            guard let scheme = url.scheme?.lowercased() else {


I do agree that this is much nicer as a user, but we're sacrificing the type of error for a "this worked or not" without the context on what the problem was. I think I'd prefer to keep the granular errors vs. checking for nil instead, but if there's already been discussion (or others have the other preference!) happy to defer.

I didn't follow all of the discussions here recently... but would it be an option to add address the above concern by adding static func validated(<mirror init's param list>) throws -> Request, users get to choose then, and if it was discussed that the nil is "nicer" then that's indeed the default still 🤔

Great call, that helps with surfacing the specific issue if someone really wants it. 🧠

I find nil-returning functions like this profoundly frustrating as they're almost impossible to debug. It's very annoying to look at something you're confident is valid data and to have the program tell you that it is not valid, without any explanation as to why.

I'd be strongly -1 on this without at least the option for a constructor that gives me better diagnostics, but frankly I'm not sure if the convenience of the nil return is worth it for the inevitable failure of debuggability down the line.

@Lukasa this PR originated as a simpler and safer way compared to #56. totally agree on the information loss point as brought up by @Yasumoto and @ktoso ^^, and think we can keep the API throwing - i was just mirroring #56 in that sense. that said, the existing throwing code has a validation/mutability race that needs to be addressed and this PR demonstrates that part

this is the throwing version: #67

artemredkin · 2019-07-10T05:29:07Z

Sources/AsyncHTTPClient/HTTPHandler.swift

        public var headers: HTTPHeaders
        public var body: Body?
+
+        internal let scheme: String
+        internal let host: String


What if user will modify the url parameter? And new url doesn't contain a host?

@artemredkin url is a let in this PR

t089 · 2019-07-10T07:30:54Z

Not sure if this was discussed already, but how about this: if the given URL is not suitable for firing off the request, instead of throwing or returning nil, create an appropriate Error and immediately fail the Promise/Task with this error? This way the API is very easy to use (no do/catch, no optional check) and in the rare case that for some reason the URL is incomplete it will be catched by the error handling that is anyhow necessary when doing network requests.

artemredkin · 2019-07-10T07:32:30Z

@t089 like in #56 ?

t089 · 2019-07-10T08:10:13Z

@artemredkin Ok, yes, sorry I got confused. Actually both PRs do this already right? The only remaining question is how to represent a validated URL as an internal type, if I understand correctly?

Lukasa

I'm not sure about this patch right now: I don't know if I consider making request non-throwing should be considered a goal at all.

Lukasa · 2019-07-10T09:23:32Z

Sources/AsyncHTTPClient/HTTPHandler.swift

-            guard let scheme = url.scheme else {
-                throw HTTPClientError.emptyScheme
+        public init?(url: URL, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) {
+            guard let scheme = url.scheme?.lowercased() else {


I find nil-returning functions like this profoundly frustrating as they're almost impossible to debug. It's very annoying to look at something you're confident is valid data and to have the program tell you that it is not valid, without any explanation as to why.

I'd be strongly -1 on this without at least the option for a constructor that gives me better diagnostics, but frankly I'm not sure if the convenience of the nil return is worth it for the inevitable failure of debuggability down the line.

Lukasa · 2019-07-10T09:24:12Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+            method = .GET
+            body = nil
+            headers.remove(name: "Content-Length")
+            headers.remove(name: "Content-Type")            


What's this whitespace about?

Lukasa · 2019-07-10T09:30:17Z

As @weissi pointed out to me, if you want to replace a function that is throws with one that returns nil, there's a keyword for that: try?. It is always possible to trivially transform a throwing function to a nil-returning one. However, it is not possible to get the throwing information back.

Right now we notify the user of what exactly we think they did wrong, and I think it's simply the wrong thing to do to hide this information.

Lukasa

I remain opposed to merging this patch, and prefer making further improvements to #67.

(I'm adding this because a re-review was requested, not because I think anyone needs the update).

Lukasa

I remain opposed to merging this patch, and prefer making further improvements to #67.

(I'm adding this because a re-review was requested, not because I think anyone needs the update).

tomerd · 2019-07-18T15:55:54Z

this is an older proposal, the discussion is happening in #67

tomerd requested review from artemredkin, ianpartridge and tanner0101 and removed request for artemredkin and ianpartridge July 10, 2019 01:27

tomerd mentioned this pull request Jul 10, 2019

refactor to make request non-throwing #56

Closed

tomerd force-pushed the non-throwing branch from d1ffddc to d38c49d Compare July 10, 2019 01:30

tomerd force-pushed the non-throwing branch from d38c49d to 43d4892 Compare July 10, 2019 01:31

tomerd requested a review from weissi July 10, 2019 01:34

Yasumoto reviewed Jul 10, 2019

View reviewed changes

artemredkin reviewed Jul 10, 2019

View reviewed changes

Lukasa requested changes Jul 10, 2019

View reviewed changes

weissi requested review from artemredkin and Lukasa July 17, 2019 09:52

Lukasa requested changes Jul 17, 2019

View reviewed changes

weissi requested a review from Lukasa July 17, 2019 10:00

Lukasa requested changes Jul 17, 2019

View reviewed changes

tomerd closed this Jul 18, 2019

make API non-throwing #66

make API non-throwing #66

Uh oh!

Conversation

tomerd commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t089 commented Jul 10, 2019

Uh oh!

artemredkin commented Jul 10, 2019

Uh oh!

t089 commented Jul 10, 2019

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Jul 10, 2019

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Jul 18, 2019

Uh oh!

Uh oh!

tomerd Jul 10, 2019 •

edited

Loading