Skip to content

MINOR: Replacing for with foreach loop in common module#2530

Closed
kashyap-prabhat wants to merge 1 commit intoapache:trunkfrom
kashyap-prabhat:refactored-code
Closed

MINOR: Replacing for with foreach loop in common module#2530
kashyap-prabhat wants to merge 1 commit intoapache:trunkfrom
kashyap-prabhat:refactored-code

Conversation

@kashyap-prabhat
Copy link
Copy Markdown
Contributor

No description provided.

@asfbot
Copy link
Copy Markdown

asfbot commented Feb 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1613/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link
Copy Markdown

asfbot commented Feb 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1616/
Test PASSed (JDK 8 and Scala 2.11).

Copy link
Copy Markdown
Member

@ijuma ijuma 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. The usage of the foreach loop looks good. However, there are a number of changes where the newline is removed after the foreach or if/else and we don't do that in Kafka's Java code (although we do in Scala's code).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The convention (even though not enforced by checkstyle) is for parent.checkForest(sensors); to go in the next line for the Java code.

@asfbot
Copy link
Copy Markdown

asfbot commented Feb 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1613/
Test FAILed (JDK 7 and Scala 2.10).

@kashyap-prabhat
Copy link
Copy Markdown
Contributor Author

@ijuma Thanks for pointing it out. Please check I have fixed it.

@asfbot
Copy link
Copy Markdown

asfbot commented Feb 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1622/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link
Copy Markdown

asfbot commented Feb 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1619/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link
Copy Markdown

asfbot commented Feb 10, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1619/
Test PASSed (JDK 7 and Scala 2.10).

Copy link
Copy Markdown
Member

@ijuma ijuma 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, LGTM. I fixed formatting in a few places before merging to trunk.

@asfgit asfgit closed this in d734f4e Feb 18, 2017
hachikuji pushed a commit to confluentinc/kafka that referenced this pull request Feb 23, 2017
Author: Prabhat Kashyap <prabhat.kashyap@knoldus.in>

Reviewers: Ismael Juma <ismael@juma.me.uk>

Closes apache#2530 from PKOfficial/refactored-code
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