-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix compiler warnings #473
Conversation
The change is acceptable as `ConsumerGroup` is the only client of `ConsumerGroup.Manager`. The defaults on `opts` parameter aren't used and their loss is thus likewise acceptable. Fixes a compiler warning due to a deprecated usage of `Supervisor.Spec.worker/3`.
This enables the usage of `child_spec/1` and removal of the deprecated `Supervisor.Spec.supervisor/3` call. Fixes kafkaex#390.
@@ -10,15 +10,15 @@ KafkaEx | |||
[![API Docs](https://img.shields.io/badge/api-docs-yellow.svg?style=flat)](http://hexdocs.pm/kafka_ex/) | |||
|
|||
KafkaEx is an Elixir client for [Apache Kafka](http://kafka.apache.org/) with | |||
support for Kafka versions 0.8.0 and newer. KafkaEx requires Elixir 1.5+ and | |||
support for Kafka versions 0.8.0 and newer. KafkaEx requires Elixir 1.6+ and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Config
as a replacement for Mix.Config
was introduced in 1.9. We would need to update this to say 1.9 if we make that change. We should also change mix.exs to require the same version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proposed version bump to 1.6+
(e513413) was meant to get README in sync with the already-changed mix.exs.
While the migration to Config
module (1f4e99a), introduced in 1.9.0, was meant to remove the warning for KafkaEx developers. This change has no bearing on KafkaEx users. Though it was my oversight that Elixir 1.6 was still used in the test suite.
Hence I think it's best if I drop 1f4e99a. A decision to bump versions from 1.6 to 1.9 can be made at some other time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuawscott Can we merge this?
start_link/1 was added in PR kafkaex#473.
start_link/1 was added in PR kafkaex#473.
The usage of
child_spec/1
addresses #390.