Skip to content
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

Translate int size values into module attributes #380

Merged
merged 2 commits into from
Nov 12, 2019

Conversation

habutre
Copy link
Contributor

@habutre habutre commented Oct 29, 2019

This commit aim to review possible uses of binary concatenation rather
than iolist concat that according with outcomes from issues #231 and #290
had proofed to be more efficient

In the end no other binary concat were found, but some small changes will
be proposed as follows:

  1. Translate literal size values to meaningful module attributes
  2. The KafkaEx.Protocol.Common module was returning a empty string instead of empty iolist on topic_data([]) fun, there is not big improvements here but a normalization among protocol modules
  3. Added missing test cases for KafkaEx.Protocol.DeleteTopics

@sourcelevel-bot
Copy link

Hello, @habutre! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

This commit aim to review possible uses of binary concatenation rather
than iolist concat that according with outcomes from issues kafkaex#231 and kafkaex#290
had proofed to be more efficient

In the end any other binary concat were found, but some small changes will
be proposed as follows:

1. The Common module was returning a empty string instead empty iolist on
`topic_data([])`, there is not big improvements here but a normalization
among protocol modules

2. Replace literal integer size values to meaningful module attributes
on Produce module
Copy link
Member

@joshuawscott joshuawscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I have one small change, otherwise it looks good!

test/protocol/delete_topics_test.exs Outdated Show resolved Hide resolved
@bjhaid
Copy link
Member

bjhaid commented Nov 11, 2019

Thanks for the PR, can you please update the title of the PR to reflect the work done in the body of the PR.

@habutre habutre changed the title Review binary concat usage Translate int size values into module attributes Nov 11, 2019
Copy link
Member

@joshuawscott joshuawscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks!

@joshuawscott joshuawscott merged commit ddb9a39 into kafkaex:master Nov 12, 2019
@joshuawscott joshuawscott mentioned this pull request Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants