Adapter: consistent returns #125
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swelham
reviewed
Mar 1, 2022
swelham
reviewed
Mar 1, 2022
| @callback deliver(%Apus.Message{}, config :: map) :: {:ok, Apus.Message.t()} | {:error, any} | ||
|
|
||
| @doc "Initialize adapter configuration." | ||
| @callback handle_config(config :: map) :: config :: map |
There was a problem hiding this comment.
Possible typo on the return type here?
I think this should be :: map instead of :: config :: map?
Comment on lines
+12
to
+15
| case SentMessages.push(message) do | ||
| :ok -> {:ok, message} | ||
| error -> {:error, error} | ||
| end |
There was a problem hiding this comment.
The SentMessages.push function always returns the same result so I don't think we need the case here.
Can we remove the case and update SentMessages.push to return {:ok, message}?
Comment on lines
+19
to
+22
| case send(self(), {:delivered_message, message}) do | ||
| {:delivered_message, _} -> {:ok, message} | ||
| error -> {:error, error} | ||
| end |
There was a problem hiding this comment.
The send function always returns the given input so we shouldn't need the case here.
As we can hardcode the response, maybe something like this would be clearer?
send(self(), {:delivered_message, message})
{:ok, message}| @@ -13,7 +13,5 @@ defmodule Apus.SentMessages do | |||
| Agent.update(__MODULE__, fn messages -> | |||
| [message | messages] | |||
| end) | |||
|
|
|||
| message | |||
There was a problem hiding this comment.
Related to the comment above on the local_adapter, could we change this to {:ok, message} to keep this clearer?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactors all adapters to return
{:ok, %Apus.Message.t()} | {:error, any()}tuples.The
LocalAdapterandTestAdapterin particular returned inconsistent responses.This does not change the SmsSender API (yet), so it is a non-breaking change to anyone calling
SmsSender.deliver/1and not calling the adapters directly.Note I'm maintaining a downstream fork here: https://gitlab.com/soapbox-pub/apus