WIP: Allow multiple header values for the same key #472
Conversation
lancejjohnson
force-pushed
the
469-multiple-headers
branch
from
affcfae
to
c9802d9
Compare
lancejjohnson
changed the title
There was a problem hiding this comment.
I took a glance through this and overall it looks good, and I like the idea of the optional :combine key that can be used if needed but otherwise the default behavior remains. I'm curious about the options for normalize_headers - is the idea that different adapters will require the multiple headers to come in different ways, and so we need to allow for the different ways of normalizing them? If so, I find myself wondering whether maybe this is trying to do too much at once, and can we introduce this with maybe the base strategy, which would resolve a certain set of cases, and then follow up with the other strategy? I don't have the full context though and not sure how this ties in with the individual adapters so not totally sure. 😄 Thanks for starting to tackle this - this is a hard problem!
lancejjohnson
force-pushed
the
469-multiple-headers
branch
from
c9802d9
to
822e262
Compare
|
|
||
| def normalize_headers(%{headers: headers}, :csv) do | ||
| Enum.reduce(headers, [], fn | ||
| {header_key, [_ | _] = header_value}, normalized -> |
There was a problem hiding this comment.
It always takes me a second to grok [_ | _] matches--would it be possible to use a guard here instead? Maybe something like this
{header_key, header_value}, normalized when is_list(header_value) ->
# ...
{header_key, header_value}, normalized ->
# ...There was a problem hiding this comment.
[_ | _] is actually a pattern match for a non-empty list. To use guards in that case would require
{header_key, header_value}, normalized when is_list(header_value) and length(header_value) > 0 ->
# ...My personal preference is the pattern match. Do you think the guard clause is an improvement?
There was a problem hiding this comment.
That's a good point. Do we need to differentiate between empty and non-empty lists? I think currently if you passed an empty list, it'd fall to the second function definition (which expects an atom or a string), and would eventually result in some other error. On the other hand, if we just looked for a list (and didn't care whether it was empty or not), the worse we'd end up doing is Enum.join([], ", "), which would just be an empty string.
At a somewhat higher level, I'm not sure whether put_header("my-header", []) is a valid operation or not. I kinda feel like it's not, but I think we should probably leave that up to the developer and treat it the same as we would put_header("my-header", ""). What do you think?
The Email struct can have multiple values added for the same name. This commit updates the `put_header` function on the Email module by allowing header values to be added as a list and combining multiple values for the same name into lists. The Mailer module also has a function to normalize those headers into a list of two element tuples to can be sent to the http client library based on the strategy elected by the caller. Resolves #469, #343, #360
lancejjohnson
force-pushed
the
469-multiple-headers
branch
from
822e262
to
508aed1
Compare
|
@lancejjohnson Was there more you wanted to do here, or is it ready to merge? (It is marked WIP but looks like you addressed most of the feedback I think?) |
There was a problem hiding this comment.
Hi @lancejjohnson 👋 , what do you think is the status of this PR?
I see that we're allowing users to set multiple headers, but how do we translate those for each adapter? Do we have a default that can work for adapters or is that work that we need to add to this PR?
Thanks for trying to handle this. I know it's a big one.
| separated string. Passing `:list` as the second argument, however, produces a | ||
| distinct tuple for every value in the header value list. | ||
| """ | ||
| def normalize_headers(%{headers: _} = email) do |
There was a problem hiding this comment.
Should we be calling this function from validate_and_normalize in line 185 from which we callnormalize_addresses?
As it stands, we've given users the ability to set different types of headers, but they have to explicitly call normalize_headers/2? Is there a default we could call? Or does the user need to determine how they send headers?
|
This PR seems to have stalled. Since I think there's more work to be done here, and this can't be merged as is (and if you don't mind @lancejjohnson), I'm going to close this so it doesn't look as though it's ready to go. If we ever loop back here, feel free to reopen and we can get this merged in. |
The Email struct can have multiple values added for the same name. This
commit updates the
put_headerfunction on the Email module by allowingheader values to be added as a list and combining multiple values for
the same name into lists. The Mailer module also has a function to
normalize those headers into a list of two element tuples to can be sent
to the http client library based on the strategy elected by the caller.
Resolves #469, #343, #360