Skip to content

Conversation

@Bentheburrito
Copy link

@Bentheburrito Bentheburrito commented Jan 4, 2025

Hello! Thanks for this lib, it's been very helpful in one of my projects as I learn Mnesia.

This PR fixes a couple of spec discrepancies dialyzer highlighted, adds support for :coerce in read/3, and adds Query.select_continue.

I hope you don't mind, I also added a .formatter.exs and formatted the project (a4214bc). Reviewing by these commits (with with actual functional changes) will be easier:

  • 591fe4e spec changes, support :coerce in read
  • bb108c2 adds select_continue

@sheharyarn
Copy link
Owner

sheharyarn commented Jan 14, 2025

Thank you for the PR!

This project was started before the Elixir formatter was created, and follows its own unique style-guide (which predates formatter). I do plan on updating the code formatting and style to use the modern formatter but that requires a bit of work from me before I'm ready to do that.

Would you be kind enough to update your PR to only include the code changes, spec tweaks, etc. without the formatting?

Thank you again!

@Bentheburrito
Copy link
Author

That makes sense - I reverted the formatting changes in the last commit. Let me know if anything else should be changed!

@Bentheburrito Bentheburrito changed the title add formatter, spec tweaks, and add select_continue spec tweaks, and add select_continue Feb 8, 2025

:match_object
|> Mnesia.call([table, pattern, lock])
|> coerce_records
Copy link

Choose a reason for hiding this comment

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

This is a breaking change, is it intentional? At least without the patch I get a map, and with the patch I get just the values as a tuple. Of course, calling Memento.Query.all(…, coerce: true) fixes this, but shouldn't the default be true here?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm definitely unintentional. Could you share a snippet that reproduces this? I'm not sure how you could get a map out of parse_results

Copy link

Choose a reason for hiding this comment

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

ah sorry, I was referring to the individual results, not the whole data structure. Here's what I mean:

defmodule Dummy do
  use Memento.Table, attributes: [:id, :title, :content]

  def setup() do
    Memento.Table.create(Dummy)

    Memento.transaction(fn ->
      Memento.Query.write(%Dummy{id: 1, title: "hi", content: "all"})
    end)
  end

  def all_no_coerce() do
    Memento.transaction(fn ->
      Memento.Query.all(Dummy, coerce: false)
    end)
  end

  def all_with_coerce() do
    Memento.transaction(fn ->
      Memento.Query.all(Dummy, coerce: true)
    end)
  end
end

# on this PR:
# iex(5)> Dummy.setup
# {:ok, %Dummy{__meta__: Memento.Table, id: 1, title: "hi", content: "all"}}
# iex(6)> Dummy.all_no_coerce
# {:ok, [{Dummy, 1, "hi", "all"}]}
# iex(7)> Dummy.all_with_coerce
# {:ok, [%Dummy{__meta__: Memento.Table, id: 1, title: "hi", content: "all"}]}

# on 0.5:
# iex(5)> Dummy.setup
# {:ok, %Dummy{__meta__: Memento.Table, id: 1, title: "hi", content: "all"}}
# iex(6)> Dummy.all_no_coerce
# {:ok, [%Dummy{__meta__: Memento.Table, id: 1, title: "hi", content: "all"}]}
# iex(7)> Dummy.all_with_coerce
# {:ok, [%Dummy{__meta__: Memento.Table, id: 1, title: "hi", content: "all"}]}

I always specified coerce in the demo, so maybe the issue is not the default value, but that PR and release version behave differently when setting coerce: false?

Copy link
Author

Choose a reason for hiding this comment

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

oh I see, thanks for the example. Yeah the default value for both all/1,2 and match/2,3 should be true just like the other functions. I'll push a fix in a bit, thanks for catching that. I don't think this is a breaking change, since it's adding a new option that wasn't supported before - giving coerce: false to all/1,2 on 0.5 isn't actually preventing the coercion (hence why you still get a struct on 0.5)

…est, fix select_raw test (breaking), fix dialyzer error (ets.continuation is opaque), use charlist sigil to avoid compile warning
@Bentheburrito
Copy link
Author

Note: this does actually contain a breaking change, since parse_result fulfilled the TODO previously in coerce_records to return the {coerced_records, continuation} tuple, instead of just the coerced_records

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