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

chore: add key_schema_id and value_schema_id to format properties #8185

Merged
merged 7 commits into from
Oct 20, 2021

Conversation

lihaosky
Copy link
Member

@lihaosky lihaosky commented Sep 28, 2021

Description

This PR depends on #8177. Review after this commit: c760b58.

Test done

  • Added new unit test.
  • Updated YATT test.

@lihaosky lihaosky changed the title refactor property refactor: move key_schema_id and value_schema_id to CommonCreateConfig Sep 28, 2021
@lihaosky lihaosky requested review from vcrfxia, mjsax and a team September 28, 2021 02:52
@lihaosky lihaosky marked this pull request as ready for review September 28, 2021 23:34
@lihaosky lihaosky force-pushed the refactor-property branch 3 times, most recently from 1589b56 to bd0b22d Compare October 2, 2021 02:25
@lihaosky lihaosky changed the title refactor: move key_schema_id and value_schema_id to CommonCreateConfig chore: move key_schema_id and value_schema_id to CommonCreateConfig and add them to format properties Oct 20, 2021
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

One nit. Overall LGTM.

As discussed in person, we should update the PR title, because moving the properties is not the main objective of the PR, but adding the schema-ids to the JSON is.

@@ -299,7 +311,12 @@ private void compare(
) {
final Object expected = extractStatement.apply(statement, config);
final Object actual = extractSource.apply(dataSource);
if (!actual.equals(expected)) {

if (actual == null && expected == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this check? IIRC Objects.equals(null,null) is a valid call and it will return true?

Nice catch on the NPE issue though!

@mjsax mjsax changed the title chore: move key_schema_id and value_schema_id to CommonCreateConfig and add them to format properties chore: add key_schema_id and value_schema_id to format properties Oct 20, 2021
@lihaosky lihaosky merged commit 84e1765 into confluentinc:master Oct 20, 2021
@lihaosky lihaosky deleted the refactor-property branch October 20, 2021 23:00
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.

2 participants