RFC: Switch from #req{} to maps #54
Conversation
Also, add more specs and breathing room here and there.
Also drop elli_request:is_request/1.
src/elli_request.erl
Outdated
| -spec version(elli:req()) -> elli_http:version(). | ||
| version(#req{version = Version}) -> Version. | ||
| -spec version(elli:req()) -> elli_http:version(). | ||
| version(Req) -> maps:get(version, Req, undefined). |
There was a problem hiding this comment.
This is a hackish workaround for the chunked_ref test in elli_tests that's expected to return {error, not_supported}, due the missing version. A better approach might be to update the test with #{version => Version} where Version =/= {1, 1}.
| args => [], | ||
| socket => undefined | ||
| }, | ||
| maps:to_list(maps:merge(Defaults, Req)). |
There was a problem hiding this comment.
I don't know how I feel about this. Is there a better way to provide defaults values for maps? We probably want to do this other places too.
There was a problem hiding this comment.
There isn't, except if always using maps:get with a default.
Part of why it may be better to stick with a record. I'm undecided on this. Are there specific benefits to using a map for req?
There was a problem hiding this comment.
Not that I can think of, except that it'd be easier to do ad hoc extensions à la Ring then.
| @@ -27,7 +27,20 @@ | |||
| -export_type([req/0, http_method/0, body/0, headers/0, response_code/0]). | |||
|
|
|||
| %% @type req(). A record representing an HTTP request. | |||
| -type req() :: #req{}. | |||
| -type req() :: #{method => elli:http_method(), | |||
There was a problem hiding this comment.
Seems wrong to have all of these being optional? I assume some are required and should be like method := elli:http_method()?
|
What's the rational for this? Are there benchmarks to see how this may alter performance? |
|
There's no particular rationale. And we don't have any such benchmarks. It's more of partial implementation of a half-baked idea. Since no one, myself include, seems particularly supportive or enthusiastic, let's table it. |
This is based on #53 and was fairly mechanical after that. Notably, it drops
elli_request:is_request/1, which, according to a quick GitHub search, no one was really using anyway.If we want to go this route, we'll probably want to add some more safeguards and/or default values. One approach can be seen in
elli_request:to_proplist/1.