[Elixir] add AnyType support #14071
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,7 +101,7 @@ defmodule {{moduleName}}.RequestBuilder do | |
| end | ||
|
|
||
| def add_param(request, :headers, key, value) do | ||
| Tesla.put_header(request, to_string(key), value) | ||
|
There was a problem hiding this comment. Given that Tesla headers (like Phoenix headers) are supposed to be all lowercase, we may want something a little more robust than just There was a problem hiding this comment. Agreed, also because Http header field names are case insensitive key
|> to_string()
|> String.downcase()Would this be sufficient, or do you mean something different by "robust" ? There was a problem hiding this comment. It’d be a start, yes, and a good one. There was a problem hiding this comment. Funny enough, this function call is actually wrong and The function should look something like this: def add_param(request, :headers, key, value) do
headers =
request
|> Map.get(:headers, [])
|> List.keystore(key, 0, {key, value})
Map.put(request, :headers, headers)
endSecondly, I wouldn't change cases as the server should/would do this to respect the case insensitivity There was a problem hiding this comment. TLDRI promised to come back, only to now postpone 2 more weeks. Shall we leave this open, or shall i create new pr's and retire this PR ? New PR proposalI'd like to address these topics in separate or in one PR:
Tesla configurabilitySeparately I'd like to use exposed middlewares in Tesla, because I think that is what Tesla's composability is about, instead of creating and running the client in it's api functions, but we'll have to decide whether that is a custom generator, an alternative generator in this repo or whether we come to the conclusion that req and tesla should be configurable settings of the same generator. The Api implementation should just compose a list of middlewares, applied to a user supplied (and defaulted) There was a problem hiding this comment. With respect to the Tesla configurability, I’d need to see more of what you think. We may be thinking similarly, but I have absolutely shifted my thinking on Tesla away from |
||
| end | ||
|
|
||
| def add_param(request, :file, name, path) do | ||
|
|
||
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’m not sure why this change was made. Yes, it saves two function calls, but it also means that it is harder to extract Tesla from the execution path in favour of https://github.com/wojtekmach/req.
(A different set of changes that I’m going to be looking at will be to replace the structs with validated maps, because there’s a lot of atoms generated in a sufficiently complex API.)
Uh oh!
There was an error while loading. Please reload this page.
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 can revert this one. It helped me with debugging the generated code.
It shows from the get-go that the pipe inputs/outputs are a
Tesla.Env.I think a good way forward with
Teslawould be to just compose a middleware stack instead of piping, which cannot be altered at runtime.I don't have experience with req, but I can look into it if you are interested in also having continued support for Tesla.
Do you want to completely do away with structs?
I concur with you on not creating atoms to the extent that they should not be generated from input (e.g. for values supplied in function calls).
Here, on the other hand, we do have a finite number of atoms, as each struct generates only it's fields' descriptors as atoms at compile time.
So IMHO this is not a case of unbounded atom generation. Ex/Beam is good at handling lots of atoms, their number should just not be unbounded or dependent on runtime and unsafe input.
Utility module for assemling middelwares ?
If you want I can look at creating a utility Module to handle a list of middlewares instead of the current approach. The Api functions would then only use this utility class to insert the necessary middlewares to set the url, params e.t.c.
This way the utility Module would allow for runtime transformations, even outside the spec (e.g. caching, delegation to other nodes e.t.c. from client libraries depending on the generated dep).
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'm not quite sure about this discussion.
Since it's not related to the implementation of
AnyType, it would be great to bring this concern into an issue. This would also allow the discussion a more retractable character.For the change itself: (by itself, I just mean the line without the changing-possibility and debugability it could bring)
I can't see the additional value it brings for breaking the style of composing/piping, and therefore I can't just let it through.
I would appreciate it if you could extract this change into a different branch and start the discussion with an issue. You could also link the new branch in the form of a PR, so it doesn't get lost.
Uh oh!
There was an error while loading. Please reload this page.
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.
Ok, will come back with a new pr, on this
EDIT
cned here: #14070 (comment)
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.
My comments on removing atoms are completely orthogonal to this discussion, just brought up as a point (I’ve hit a very large OpenAPI spec that makes a lot more than I’m comfortable with, but yes—I want to explore how structs get handled).