-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove warnings about missing EEx #66
base: master
Are you sure you want to change the base?
Conversation
mix.exs
Outdated
@@ -1,4 +1,4 @@ | |||
defmodule Benchfella.Mixfile do | |||
defmodule Benchfella.Mixfile 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 would humbly advice to run mix format mix.exs
, commit and git rebase -i
, then force-push into your PR branch to fix this excessive indentation.
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.
OMG how did that happen :(
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.
fix coming up
@@ -16,7 +16,10 @@ defmodule Benchfella.Mixfile do | |||
end | |||
|
|||
def application do | |||
[applications: []] | |||
[ | |||
applications: [], |
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 entirely sure that with newer Elixirs applications:
is obligatory even.
Consider [a patch that fixes similar issue in inch_ex.
I would just leave extra_applications: [:eex]
. I think it would be wise to also verify why does this supress the warning.
My hypothesis is that it has to do with the fact that newer Elixirs now know how to assemble minimal releases, but need a little help with figuring out which bits and bobs from the full Elixir distribution to include, but I'm not 💯% sure...
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 do not know I very simply did what the warning said 🤷
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.
but looking at the link you provided I would say they just did the same, no?
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.
Not quite. Note how inch_ex dropped applications
key entirely.
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.
you can do that too of course I will not complain, is there any action on my side yet?
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.
@RobertDober I don't think so, now we wait for the maintaners, hah.
This should us rid of