Skip to content

Add MissingDependenciesError exception #13689

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

Merged
merged 7 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions lib/elixir/lib/exception.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2131,6 +2131,56 @@ defmodule UnicodeConversionError do
end
end

defmodule MissingApplicationsError do
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, we have gone through some many bikesheds. I think last we talked it was meant to be singular? MissingApplicationError? I am fine either way, we should just double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

We agreed on plural IIRC, because we have a list of apps in the exception?

Copy link
Member

Choose a reason for hiding this comment

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

I thought some found it weird because we don’t have many plurals in module names. Your call!

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it's still the singular Error so let's go with MissingApplicationsError 😄

@moduledoc """
An exception that is raised when an application depends on one or more
missing applications.

This exception is used by Mix and other tools. It can also be used by library authors
when their library only requires an external application (like a dependency) for a subset
of features.

The fields of this exception are public. See `t:t/0`.

*Available since v1.18.0.*

## Examples

unless Application.spec(:plug, :vsn) do
raise MissingApplicationsError,
description: "application :plug is required for testing Plug-related functionality",
apps: [{:plug, "~> 1.0"}]
end

"""

@moduledoc since: "1.18.0"

@type t() :: %__MODULE__{
apps: [{Application.app(), Version.requirement()}, ...],
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned earlier that a potential use case for this is raising on missing Elixir/OTP apps. If so, apps being non-empty makes it a bit awkward imo:

raise MissingApplicationsError,
        description: "please add :ssl to :extra_applications",
        apps: [{:ssl, ">= 0.0.0"}]
** (MissingApplicationsError) please add :ssl to :extra_applications

To address this, include these applications as your dependencies:

  {:ssl, ">= 0.0.0"}

It's OK if this exception is not intended for that. Alternatively, allow :apps to be empty?

Copy link
Member

Choose a reason for hiding this comment

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

We can detect if the application comes from OTP and change the message accordingly. So no concerns here.

description: String.t()
}

defexception apps: [], description: "missing applications found"

@impl true
def message(%__MODULE__{apps: apps, description: description}) do
# We explicitly format these as tuples so that they're easier to copy-paste
# into dependencies.
formatted_apps =
Enum.map(apps, fn {app_name, requirement} ->
~s(\n {#{inspect(app_name)}, "#{requirement}"})
end)

"""
#{description}

To address this, include these applications as your dependencies:
#{formatted_apps}\
"""
end
end

defmodule Enum.OutOfBoundsError do
@moduledoc """
An exception that is raised when a function expects an enumerable to have
Expand Down
15 changes: 15 additions & 0 deletions lib/elixir/test/elixir/exception_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -954,6 +954,21 @@ defmodule ExceptionTest do
test "ErlangError" do
assert %ErlangError{original: :sample} |> message == "Erlang error: :sample"
end

test "MissingApplicationsError" do
assert %MissingApplicationsError{
apps: [{:logger, "~> 1.18"}, {:ex_unit, Version.parse_requirement!(">= 0.0.0")}],
description: "applications are required"
}
|> message == """
applications are required

To address this, include these applications as your dependencies:

{:logger, "~> 1.18"}
{:ex_unit, ">= 0.0.0"}\
"""
Copy link
Member

Choose a reason for hiding this comment

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

Here is my suggestion for the raising:

unless Application.spec(:foo_bar, :vsn) do
  raise MissingApplicationError,
        description: "application :foobar is required for feature xyz",
        apps: [{:foobar, "~> 1.0"}]
end

and it should raise:

** (MissingApplicationError) application :foobar is required for feature xyz
To address this, include this application as your dependency:

    {:foobar, "~> 1.0"}

I would keep them as tuples, because it is easier to copy and paste.

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 would keep them as tuples, because it is easier to copy and paste.

Good call.

end
end

describe "error_info" do
Expand Down