-
Notifications
You must be signed in to change notification settings - Fork 185
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
Warn on unknown dependency options #1027
Conversation
I initally considered erroring which I think would be fine for top-level dependencies, the users would easily fix them. But I'm concerned if the warning would happen in a transitive dependency it is less easy to fix and would cause disruption. The warning can be easy to miss, however. I also considered doign this in Elixir (in |
lib/hex/scm.ex
Outdated
@@ -125,6 +125,15 @@ defmodule Hex.SCM do | |||
repo = opts[:repo] || "hexpm" | |||
path = cache_path(repo, name, lock.version) | |||
|
|||
unknown_options = Keyword.keys(opts) -- ~w[hex dest repo lock env build optional]a |
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.
This is the full list of options mix supports: https://hexdocs.pm/mix/Mix.Tasks.Deps.html#module-dependency-definition-options.
lib/hex/scm.ex
Outdated
|
||
if unknown_options != [] do | ||
Hex.Shell.warn( | ||
"#{name} dependency is using unknown options: " <> |
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.
This is a very minor detail, but I'm wondering if this chosen phrasing has a (small) chance of being misunderstood?
As in, perhaps a phrase like "ex_doc dependency is using unknown options" could be understood to mean "a dependency of ex_doc is using unknown options", because the expression "ex_doc dependency" by itself could easily mean "a dependency of ex_doc".
To make the phrasing here more unambiguous, perhaps this could be worded "The dependency #{name} is using unknown options", which leaves no room for interpretation 🙂
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.
Good call, updated!
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 went ahead and change it one more time so it's consistent with another similar message and right now we have:
ex_doc is missing its version requirement, use \">= 0.0.0\"
ex_doc is using unknown options: :dir, :typo
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.
Awesome! 👏 Looks great. Good call on refining the message further 🙂
@@ -116,6 +116,16 @@ defmodule Hex.SCM do | |||
end | |||
end | |||
|
|||
# https://hexdocs.pm/mix/Mix.Tasks.Deps.html#module-dependency-definition-options |
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.
This comment 👍
Closes #1024