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

Add persistent subscription info support #76

Closed kristofka closed 1 year ago

kristofka commented 1 year ago

I've tried to abide by the coding standards. However, I'm not sure how and where to implement the tests.

Let me know how I can improve this request.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 1b9d5ff1d1affe37c4e245a77d9c7f7092bac9f5-PR-76


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/spear/persistent_subscription/info.ex 6 7 85.71%
<!-- Total: 6 7 85.71% -->
Totals Coverage Status
Change from base Build 4a77a76dd1078cdb1e2182a72ce7d7fc3f1d6e93: -0.2%
Covered Lines: 648
Relevant Lines: 649

💛 - Coveralls
the-mikedavis commented 1 year ago

This is looking good 😀

I think this just needs a test in test/spear_test.exs (around L292 or within that describe block):

@tag compatible(">= 22.10.0")
test "info about a psub can be fetched", c do
  stream = c.stream_name
  group = uuid_v4()
  settings = %Spear.PersistentSubscription.Settings{}

  assert Spear.create_persistent_subscription(c.conn, stream, group, settings) == :ok

  assert {:ok, %Spear.PersistentSubscription.Info{event_source: ^stream, group_name: ^group}} =
           Spear.get_persistent_subscription_info(c.conn, stream, group)
end
kristofka commented 1 year ago

Thanks for the pointer regarding the tests. I've made the code more consistent with other persistent_subscription functions and insured it worked well with the :all stream. I've added a few tests as per your comment.

If you feel more work is needed, feel free to let me know.

kristofka commented 1 year ago

Oops, had a couple of mistakes in the tests. Should be fixed. Sorry about that.

kristofka commented 1 year ago

I'm not sure how I've broken two unrelated tests. Could it be the CI runner? I'm a bit lost here, sorry.

the-mikedavis commented 1 year ago

Oh hmm I think those two are a little flaky. Let me give them a restart

kristofka commented 1 year ago

Reformatted spear.ex - mix format is adament that case clauses should be separated by an empty line. Should be fixed. Thank you for your patience.

the-mikedavis commented 1 year ago

Looks good, thanks! I'll check out the other new RPCs and see if we can bundle all of the new ones into a new minor release

the-mikedavis commented 1 year ago

This is now released in v1.2.0 🎉

kristofka commented 1 year ago

Thank you for your work and help. Much appreciated!