Skip to content
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

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

wojtekmach
Copy link
Member

Closes #1024

@wojtekmach
Copy link
Member Author

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 Mix.Dep.Loader). However the way the contract is set up, Mix.SCM.accepts_options(app, options) returns the options that were given to it, I don't think there's a way to automatically warn unless we change the contract.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib/hex/scm.ex Outdated

if unknown_options != [] do
Hex.Shell.warn(
"#{name} dependency is using unknown options: " <>
Copy link
Contributor

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 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, updated!

Copy link
Member Author

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

Copy link
Contributor

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment 👍

@wojtekmach wojtekmach merged commit 385e63f into main May 20, 2024
24 checks passed
@wojtekmach wojtekmach deleted the wm-deps-get-warn-unknown-options branch May 20, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reject unknown keys when calling deps.get
3 participants