FundingCircle / jackdaw

A Clojure library for the Apache Kafka distributed streaming platform.
https://fundingcircle.github.io/jackdaw/
BSD 3-Clause "New" or "Revised" License
369 stars 80 forks source link

Use jd/map->Properties on kafka-streams opts #253

Open madstap opened 4 years ago

madstap commented 4 years ago

This seems like an oversight, I found it confusing that I had to use string keys.

Checklist

Not sure how to go about testing this?

codecov[bot] commented 4 years ago

Codecov Report

Merging #253 into master will increase coverage by 0.08%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   81.16%   81.25%   +0.08%     
==========================================
  Files          41       41              
  Lines        2586     2560      -26     
  Branches      153      149       -4     
==========================================
- Hits         2099     2080      -19     
+ Misses        334      331       -3     
+ Partials      153      149       -4     
Impacted Files Coverage Δ
src/jackdaw/streams.clj 82.92% <100.00%> (-0.28%) :arrow_down:
src/jackdaw/test/commands/write.clj 92.85% <0.00%> (-0.33%) :arrow_down:
src/jackdaw/test/serde.clj 94.52% <0.00%> (-0.08%) :arrow_down:
src/jackdaw/test/transports/rest_proxy.clj 84.61% <0.00%> (-0.08%) :arrow_down:
src/jackdaw/test/transports/kafka.clj 86.04% <0.00%> (+0.43%) :arrow_up:
src/jackdaw/test/transports/mock.clj 84.48% <0.00%> (+2.73%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update daa8387...c050c83. Read the comment docs.

cddr commented 3 years ago

LGTM. The existing integration tests should cover this change.

e.g. this one which uses the kafka-streams function through the test fixture https://github.com/FundingCircle/jackdaw/blob/master/test/jackdaw/test/transports/kafka_test.clj#L54

madstap commented 3 years ago

Is there anything missing on my end for getting this PR merged?

madstap commented 2 years ago

I suppose this isn't really high priority, but the PR has seen it's first birthday already. Any hope of getting it merged?