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

Implement java.util.function.Consumer. #3916

Merged
merged 1 commit into from Jan 7, 2020

Conversation

mliarakos
Copy link
Contributor

This PR implements java.util.function.Consumer. I implemented the class based on the Java 8 Javadocs and did not look at any existing implementations. The tests are patterned after the existing Scala.js tests for Predicate.

The motivation for implementing the class is that it is required by the Akka.js CircuitBreaker, which is required to implement circuit breakers in Lagom.js.

@sjrd
Copy link
Member

sjrd commented Jan 7, 2020

Thanks for the PR. Could you please target it to the 0.6.x branch instead of master? We periodically merge 0.6.x into master, but never the other way around.

  1. Change the target branch of the PR (at the top of this page)
  2. Rebase your commit on top of 0.6.x, taking care not to carry over any other commit from the master branch
  3. Force push

makeConsumer[Any](x => throw new AssertionError(s"dontCallConsumer.accept($x)"))

assertThrows(classOf[ThrowingConsumerException],
throwingConsumer.andThen(dontCallConsumer).accept(0))
Copy link
Member

Choose a reason for hiding this comment

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

This line should have 4 spaces of indentation instead of 2.

throwingConsumer.andThen(dontCallConsumer).accept(0))

assertThrows(classOf[ThrowingConsumerException],
add.andThen(throwingConsumer).accept(1))
Copy link
Member

Choose a reason for hiding this comment

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

This line should have 4 spaces of indentation instead of 2.

@mliarakos mliarakos changed the base branch from master to 0.6.x January 7, 2020 13:31
@sjrd
Copy link
Member

sjrd commented Jan 7, 2020

Thanks.

The CI got confused because you changed the base branch only after force-pushing. You can fix the situation by doing a no-op git commit --amend then force-pushing again :)

Copy link
Member

@sjrd sjrd 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 updates!

I just noticed two blank lines that should not be there according to our coding style. Sorry, I didn't see them earlier while reviewing on my mobile.

Could you remove them and amend your commit? After that it's good to go.

}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This blank line should not exist. (There should be one New Line at the end of the file, but no empty line.)

}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This blank line should not exist. (There should be one New Line at the end of the file, but no empty line.)

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thank you :)

@sjrd sjrd merged commit 368e57c into scala-js:0.6.x Jan 7, 2020
@mliarakos
Copy link
Contributor Author

Glad to contribute, thanks for reviewing.

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.

None yet

2 participants