elixir-ecto / ecto_sql

SQL-based adapters for Ecto and database migrations
https://hexdocs.pm/ecto_sql
Apache License 2.0
578 stars 312 forks source link

Clarify that index names can be a string or an atom #639

Closed PragTob closed 1 month ago

PragTob commented 1 month ago

Definitely both work, I've tried it out to make sure. Not having it specified caused some confusion in my team and I think it can help to specify this.

I did change the time spec, as I'm rather sure it also ends up in there as a string.

I'm a bit surprised/confused that the docs default to atom usage as as best I can tell the index is used for instance here:

(Postgres connection)

    queries = [
        [
          "CREATE ",
          if_do(index.unique, "UNIQUE "),
          "INDEX ",
          if_do(index.concurrently, "CONCURRENTLY "),
          if_do(command == :create_if_not_exists, "IF NOT EXISTS "),
          quote_name(index.name),
          " ON ",
          more stuff

which ends up converting the atom back to a string:

    defp quote_name(nil, name), do: quote_name(name)

    defp quote_name(prefix, name), do: [quote_name(prefix), ?., quote_name(name)]

    defp quote_name(name) when is_atom(name) do
      quote_name(Atom.to_string(name))
    end

    more stuff

But yeah, I don't know as well - I was thinking maybe to use name as a string in the docs at least once to show it's possible but maybe that breaks with a desire for uniformity.

As always, thank you all for your wonderful work! :green_heart:

josevalim commented 1 month ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: