Use std.http.Method for Request.method #72
Conversation
|
that's my guy |
|
LGTM! |
examples/hello_json/hello_json.zig
Outdated
| @@ -7,8 +7,7 @@ const User = struct { | |||
| }; | |||
|
|
|||
| fn on_request(r: zap.Request) void { | |||
| if (!std.mem.eql(u8, r.method.?, "GET")) | |||
| return; | |||
| if (r.method == null or r.method.? == .GET) return; | |||
There was a problem hiding this comment.
this should be != .GET
I am not sure if I like having to check 2 things instead of one
There was a problem hiding this comment.
this should be != .GET
yes, that is a bug
I am not sure if I like having to check 2 things instead of one
r.method.? should be checked anyways, I think
src/request.zig
Outdated
| @@ -266,7 +266,8 @@ pub const CookieArgs = struct { | |||
| path: ?[]const u8, | |||
| query: ?[]const u8, | |||
| body: ?[]const u8, | |||
| method: ?[]const u8, | |||
| method: ?std.http.Method, | |||
There was a problem hiding this comment.
I propose that the optional makes no sense here. I think an enum (default) value of "invalid" or sth similar makes more sense.
There was a problem hiding this comment.
fck. does std.http.Method support indicating it wasn't possible to parse the method?
There was a problem hiding this comment.
Hmmm. What about defining our own enum? WDYT? It could even be based on std.http.Method, but add a value for "invalid". std.http returns error.HttpHeadersInvalid which we can't do so we might as well put an invalid field into the enum.
There was a problem hiding this comment.
that would make switching more convenient, no need for checking != null
There was a problem hiding this comment.
I was thinking about a custom enum, but I went with std.http.Method simply because it's easier. I agree that .INVALID would be nice, and I will add that.
There was a problem hiding this comment.
Or should it be called .Custom or .Other, cuz it's not entirely impossible that someone needs to define a custom HTTP method for whatever they are working on.
There was a problem hiding this comment.
Maybe even Unexpected? Because we expect a fixed set and did not detect any of those. So it comes unexpected to zap. Or just NonStandard?
src/zap.zig
Outdated
| if (std.mem.eql(u8, m, "OPTIONS")) | ||
| return Method.OPTIONS; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
here is where the invalid would go
|
I made changes accordingly, please take a look. I named the unexpected http method |
There was a problem hiding this comment.
That seems really nice! I think that's a winner!
RE always checking the method string: In the example, we're only interested in .GET which we'd only get if the method string was "GET". Now, with known methods being an enum, the only time when to check if there is a method string is on .UNKNOWN. Then you can check if it's really something unknown or if it's just not present (which should never happen, I suppose).
| @@ -1,3 +1,5 @@ | |||
| const std = @import("std"); | |||
|
|
|||
There was a problem hiding this comment.
I haven't checked but it seems like this isn't used anymore and can go.
There was a problem hiding this comment.
It is used at line 123 ~ 133 for mapping http method string provided by Facil to Zig enum.
|
Once you have checked the potentially unused import above, I think this can be merged. I am, once again, on a train across Germany... Since this is a breaking API change, I'll have to make a new release. I am not sure if I can get that done before Friday CET because today and tomorrow are quite long and busy. I hope you don't mind. Oh, and: THANK YOU for this great contribution! Makes zap even more zig 😄 ! |
I think it is used, check the reply.
I don't mind, I can use my own alpha build for the time being.
No, THANK YOU for this amazing package, glad I can help. I love Zig, I love Zap, and have a nice trip! |
|
Merged on the phone 😎 |
|
😎 |
This PR does the following:
Request.methodbecomes anstd.http.Method enumso developers can use switch statements and other better syntax on itRequest.method_stris the original string representation of the http methodWhy both?
Quality of life. Since there is no easy and concise way of printing Enum in Zig, one will make request easier to compare and the other will be easier to print. Examples also have been updated accordingly. This decision was made based on my personal need without approval of anyone, so please do feel free to reject this PR, request changes, or make criticisms if this change doesn't feel fit for the project.
What happens if the HTTP method is none of the one listed?
Well, it shouldn't happen, because I did see a part of Zap's (or facil's I can't remember) code telling the client what http methods are acceptable. If exception does happen (edge cases matter - zig zon),
Request.methodwill be null andRequest.method_strstays the same. The Method Enum defines a set of "normal circumstances", and a null value represents exception, which I think is a good abstraction of what is to be done with http methods.This has been tested as part of my own project, and everything is working as intended. A test package is available, use at your own risk.
Closes #67