NFIBrokerage / spear

A sharp EventStoreDB v20+ client backed by Mint :yum:
https://hex.pm/packages/spear
Apache License 2.0
85 stars 14 forks source link

fix: `Spear.append/3` `raw?: true` raw return #97

Closed byu closed 4 months ago

byu commented 4 months ago

Context

Issue #96

Passing a raw?: true option to Spear.append/3 or Spear.Client/append continued to return :ok instead of {:ok, AppendResp.t()}.

This contradicted the docs and the Spear.append_batch/5 behaviour.

Solution

This commit adds the conditional case in Spear.append/3 to return {:ok, AppendResp.t()} when raw? is true.

Spear.Client.append will also be fixed as a result.

Additional Notes

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build a291c791a6e7786e9868d68811a6871b39058db7-PR-97

Details


Totals Coverage Status
Change from base Build a08dfa90d210ded99dbe5dbcbc9e0fc0f3af0996: 0.0%
Covered Lines: 711
Relevant Lines: 711

💛 - Coveralls
byu commented 4 months ago

I dev'd against image: eventstore/eventstore:24.2.0-alpha-arm64v8

I copy-pasta'd from other append_batch test...

so, those errors between 20.10.2 vs 22.6 "Compatibility / Bless"? Maybe I misunderstood compatibility, and should've copied in the tag too?

    @tag compatible(">= 21.6.0")
    test "append_batch/5 appends a batch of events", c do
      assert {:ok, batch_id, request_id} =
               random_events()
               |> Stream.take(5)
               |> Spear.append_batch(c.conn, :new, c.stream_name, expect: :empty)

Also, not sure why "CI / Bless" is saying test/spear_test.exs is failing the format?

A local mix format is showing spear_test.exs unchanged from what i PR'd?

the-mikedavis commented 4 months ago

The formatting might be caused by changes to mix format in newer Elixir versions. I have Elixir/Erlang pinned at relatively old versions in https://github.com/NFIBrokerage/spear/blob/main/.tool-versions

I think it would be ok to bump those but it should be a separate change from this fix

the-mikedavis commented 4 months ago

Yeah the compatibility checks are there to make sure we can run against older EventStoreDB releases and with older Elixir/Erlang versions. I believe the batch append RPC was introduced in 21.6.0 so we can't run tests against it on all versions of the matrix