coryodaniel / arbor

Ecto elixir adjacency list and tree traversal. Supports Ecto versions 2 and 3.
MIT License
239 stars 26 forks source link

Ecto struct_fields #35

Closed wwww-wwww closed 2 years ago

wwww-wwww commented 2 years ago

https://github.com/coryodaniel/arbor/blob/1d3c610f4c1cc334af332aa11516cf7d543d2209/lib/arbor/tree.ex#L39

It is now :ecto_struct_fields in Ecto 3.8

Although switching to :ecto_struct_fields would also imply ecto >= 3.8 and elixir ~> 1.10

durub commented 2 years ago

What if we try to fetch from :ecto_struct_fields and, if nil, we try to fetch from :struct_fields?

We would keep compatibility with previous versions of Ecto while also supporting >= 3.8 (and not requiring Elixir ~> 1.10 if they are using Ecto <= 3.8).

Would that work? Seems like a simple fix to make.

Something like this:

struct_fields = Module.get_attribute(definition, :ecto_struct_fields) || Module.get_attribute(definition, :struct_fields)

Or, more explicitly:

struct_fields =
  if Module.has_attribute?(definition, :ecto_struct_fields) do
    # Ecto >= 3.8
    Module.get_attribute(definition, :ecto_struct_fields)
  else
    Module.get_attribute(definition, :struct_fields)
  end

If that doesn't work by some reason I'm not seeing, the README already has instructions for Ecto 2 and Ecto 3. Maybe having a version bump for >= Ecto 3.8 wouldn't be so bad...

What do you folks think, @wwww-wwww and @coryodaniel? Our project requires >= Ecto 3.8 due to another dependency and I'm currently vendoring arbor with this fix.

If we decide on an approach, I can make a PR so we can fix this.

coryodaniel commented 2 years ago

I think that sounds like a good approach. Are there any other 3.8 issues you’ve run into?

wwww-wwww commented 2 years ago

I haven't run into any issues yet.