Skip to content

Conversation

wojtekmach
Copy link
Contributor

This is similar to sneako/finch@6b42c8e

While at it I made the following change which fixes what I think was unintentional ommision of interpolation:

-  defp to_name(name), do: :"__MODULE__:#{name}"
+  defp to_name(name), do: :"#{inspect(__MODULE__)}:#{name}"

I also noticed we have:

def start_link(opts) when is_list(opts) do
  with {:ok, name} <- Keyword.fetch(opts, :name) do
    GenServer.start_link(__MODULE__, opts, name: to_name(name))
  end
end

And so if we do this, i.e. don't pass :name, we'll get :error

iex> Meilisearch.start_link([])
:error

That is not a common return value from start_link. Supervisor will crash on it. So while the child_spec/1 change that I am proposing is technically a breaking change I think it is warranted as the current behaviour under that circumstance, not passing name, was also crashing.

In this spirit, my suggestion is to change start_link, something like:

   def start_link(opts) when is_list(opts) do
-    with {:ok, name} <- Keyword.fetch(opts, :name) do
-      GenServer.start_link(__MODULE__, opts, name: to_name(name))
-    end
+    name = Keyword.fetch!(opts, :name)
+    GenServer.start_link(__MODULE__, opts, name: to_name(name))
   end

Which I'm happy to do. let me know!

This is similar to sneako/finch@6b42c8e

While at it I made the following change which fixes what I think was
unintentional ommision of interpolation:

    -  defp to_name(name), do: :"__MODULE__:#{name}"
    +  defp to_name(name), do: :"#{inspect(__MODULE__)}:#{name}"

I also noticed we have:

    def start_link(opts) when is_list(opts) do
      with {:ok, name} <- Keyword.fetch(opts, :name) do
        GenServer.start_link(__MODULE__, opts, name: to_name(name))
      end
    end

And so if we do this, i.e. don't pass :name, we'll get :error

    iex> Meilisearch.start_link([])
    :error

That is not a common return value from start_link. Supervisor will crash
on it. So while the `child_spec/1` change that I am proposing is
technically a breaking change I think it is warranted as the current
behaviour under that circumstance, not passing name, was also crashing.

In this spirit, my suggestion is to change start_link, something like:

       def start_link(opts) when is_list(opts) do
    -    with {:ok, name} <- Keyword.fetch(opts, :name) do
    -      GenServer.start_link(__MODULE__, opts, name: to_name(name))
    -    end
    +    name = Keyword.fetch!(opts, :name)
    +    GenServer.start_link(__MODULE__, opts, name: to_name(name))
       end

Which I'm happy to do. let me know!
@BlitzBanana
Copy link
Member

Hey @wojtekmach, thanks for your contribution.
I'm OK to raise when no name is supplied, you can make the change.

@wojtekmach
Copy link
Contributor Author

Done!

@BlitzBanana BlitzBanana merged commit f8c787f into nutshell-lab:main Feb 14, 2025
3 checks passed
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.

2 participants