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

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

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

PragTob
Copy link
Contributor

@PragTob PragTob commented Oct 7, 2024

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! 💚

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)
```elixir
    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:

```elixir
    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.
Copy link
Member

@greg-rychlewski greg-rychlewski left a comment

Choose a reason for hiding this comment

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

I think this is correct. @josevalim are you also ok advertising both string and atom?

@josevalim josevalim merged commit e57c91a into elixir-ecto:master Oct 22, 2024
10 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

dkuku pushed a commit to dkuku/ecto_sql that referenced this pull request Nov 4, 2024
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.

3 participants