Add Elixir 1.6 to travis matrix #277
Conversation
I also trimmed down the number of builds so that we only run a single build for each version of Elixir. I found 2 credo issues and fixed them. I would be in favor of running credo and dialyzer in travis, but would like a second opinion on that. Also updated some dependencies
|
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/kafkaex/kafka_ex/pulls/277. |
dantswain
left a comment
dantswain
left a comment
There was a problem hiding this comment.
I used to try to keep the most common version combos in the build matrix - e.g., people were using 1.5 with R19 and R20. I guess it's probably OK to just test the more recent versions and add others if anyone actually finds an issue.
Personally I prefer var = %Struct{} matching. I know this was throwing credo warnings but I had been sort of slowly trying to switch things when I was in that part of the code. I guess it's better to have a clean build than to have it match my preference 🤷♂️
| correlation_id, | ||
| client_id, | ||
| request = %Request{compression: compression, messages: messages} | ||
| %Request{compression: compression, messages: messages} = request |
There was a problem hiding this comment.
I personally prefer the other way - var = %Struct{...}... credo just looks for consistency iirc?
There was a problem hiding this comment.
It does look for consistency, but I think the elixir standard library always does %Struct{} = var.
There was a problem hiding this comment.
🤷♂️ I don't care either way, but I did switch a bunch to the %Struct{} = var in #232 😱
Add Elixir 1.6 to travis matrix
I also trimmed down the number of builds so that we only run a single
build for each version of Elixir.
I found 2 credo issues and fixed them. I would be in favor of running
credo and dialyzer in travis, but would like a second opinion on that.
Also updated some dependencies