Elixir 1.11 warnings #556
Elixir 1.11 warnings #556
Conversation
There was a problem hiding this comment.
Thanks for opening a PR trying to fix Elixir 1.11 warnings.
I had a few questions, and could you provide more information for the changes you're making? I haven't run through Bamboo with Elixir 1.11, so I'm not 100% sure what warnings the changes correspond to.
| worker(Bamboo.SentEmail, []), | ||
| supervisor(Task.Supervisor, [[name: Bamboo.TaskSupervisorStrategy.supervisor_name()]]) | ||
| Bamboo.SentEmail, | ||
| {Task.Supervisor, [name: Bamboo.TaskSupervisorStrategy.supervisor_name()]} |
| def deliver(email, %{open_email_in_browser_url: open_email_in_browser_url}) do | ||
| %{private: %{local_adapter_id: local_adapter_id}} = SentEmail.push(email) | ||
| open_url_in_browser("#{open_email_in_browser_url}/#{local_adapter_id}") | ||
| end | ||
|
|
||
| @doc "Adds email to `Bamboo.SentEmail`" |
There was a problem hiding this comment.
Add of curiosity, why remove the doc? Could we revert changes to this?
There was a problem hiding this comment.
Elixir 1.11 throws a warning for this behavior. You can't add two different calls to @doc for the same function, you have to condense it into a single place.
| {:ex_doc, "~> 0.20", only: :dev}, | ||
| {:excoveralls, "~> 0.13", only: :test}, | ||
| {:floki, "~> 0.29", only: :test}, | ||
| {:ex_doc, "~> 0.23", only: :dev}, |
There was a problem hiding this comment.
Could you provide information as to why we needed these changes? Did we have to update dependencies?
There was a problem hiding this comment.
Force of habit, since all of these deps are limited to :dev and :test there should not be any issues with updating.
There was a problem hiding this comment.
We add :mime since it is used in lib/bamboo/attachment.ex and not directly referenced.
There was a problem hiding this comment.
We mark phoenix as optional since it is used in lib/bamboo/phoenix.ex with Phoenix.View.render_to_string. It's not 100% necessary to use that module but it's very likely to be used.
|
Hi @germsvel, would it be possible to get this merged? |
mix.exs
Outdated
| @@ -21,7 +21,8 @@ defmodule Bamboo.Mixfile do | |||
| start_permanent: Mix.env() == :prod, | |||
| package: package(), | |||
| docs: [main: "readme", extras: ["README.md"]], | |||
| deps: deps() | |||
| deps: deps(), | |||
| xref: [exclude: [IEx, ExMachina]] | |||
There was a problem hiding this comment.
Hi @tomciopp,
Thanks so much for all the extra information you gave regarding the dependencies. I was looking through this PR again, and this caught my eye. Could you give some extra info as to why we're excluding these from xref?
When I read the Elixir 1.11 announcement, it seems like these two should not be excluded here.
It seems like iex should fall under the "part of Elixir" portion of libraries, so we should put it in extra_applications, right? Or to dig even further, do we even need to list IEx at all? Where does Bamboo use IEx?
And ExMachina is defined in deps, so it seems like we shouldn't need to list it here either, right?
Thanks for all the info on the rest of the PR. I think this is the last question I have.
There was a problem hiding this comment.
If I'm not mistaken, adding :iex to :extra_applications would imply that IEx should be started whenever Bamboo starts, which I don't think is the desired behavior.
As for ExMachina, I'm not sure why that needed to be excluded.
There was a problem hiding this comment.
IEx is used in the following file
lib/mix/start_sent_email_viewer_task.ex:70
Here is the warning emitted
warning: IEx.started?/0 defined in application :iex is used by the current application but the current application does not directly depend on :iex. To fix this, you must do one of:
1. If :iex is part of Erlang/Elixir, you must include it under :extra_applications inside "def application" in your mix.exs
2. If :iex is a dependency, make sure it is listed under "def deps" in your mix.exs
3. In case you don't want to add a requirement to :iex, you may optionally skip this warning by adding [xref: [exclude: IEx]] to your "def project" in mix.exs
We don't want IEx to be started when Bamboo starts so we use option 3 to exclude it.
There was a problem hiding this comment.
There is a separate warning with ExMachina, but I believe it is related to the library itself. I will remove the module from the xref exclude list. I've reproduced the warning below for your convenience.
warning: incompatible types:
map() !~ Bamboo.NormalizeAndPushStrategy
in expression:
# test/support/normalize_and_push_strategy.ex:2
custom_strategy_module.function_name
where "custom_strategy_module" was given the type Bamboo.NormalizeAndPushStrategy in:
# test/support/normalize_and_push_strategy.ex:2
custom_strategy_module = Bamboo.NormalizeAndPushStrategy
where "custom_strategy_module" was given the type map() (due to calling var.field) in:
# test/support/normalize_and_push_strategy.ex:2
custom_strategy_module.function_name
HINT: "var.field" (without parentheses) implies "var" is a map() while "var.fun()" (with parentheses) implies "var" is an atom()
Conflict found at
test/support/normalize_and_push_strategy.ex:2: Bamboo.NormalizeAndPushStrategy.__using__/1
Fixes warnings emitted by the compiler and updates supervisors to new methodology.