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

Ikc 208 json registry support #254

Merged
merged 4 commits into from
May 20, 2022
Merged

Conversation

juris-97
Copy link
Contributor

No description provided.

Copy link
Contributor

@matty-matt matty-matt left a comment

Choose a reason for hiding this comment

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

Dlaczego zmiana enuma z JSON_SCHEMA na JSON?

@juris-97
Copy link
Contributor Author

juris-97 commented May 20, 2022

@matty-matt bo:
image
leciał wyjątek w kouncil że takiego typu MessageFormat.JSON nie istnieje

@juris-97 juris-97 requested a review from matty-matt May 20, 2022 08:15
}

@Test
public void should_deserialize_without_schema() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tak w zasadzie to po co jest ten test i dwa kolejne? Widzę, że są one też w klaskach do deserializacji dla avro i protobuf. Różnią się czymś między sobą? Może warto by było je gdzieś wydzielić albo zostawić tylko w jednej klasie?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

też zastanawiałem się żeby zrobić jedną klasę, która miałaby wydzielone wspólne rzeczy i wtedy każda by dziedziczyła od niej, to może tak zrobię

Copy link
Contributor

Choose a reason for hiding this comment

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

Bardziej myślałem żeby te dwa czy trzy testy do osobnej klasy przerzucić

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, wydzieliłem te bez scheme'y do dwóch osobnych klasek (ser/des)

@juris-97 juris-97 requested a review from Pefes May 20, 2022 09:08
assertThat(deserializedMessage.getValueData().getMessageFormat()).isNull();
}

private ConsumerRecord<Bytes, Bytes> prepareConsumerRecord(Bytes key, Bytes value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Może jescze to do jakiegoś utila? Czy to już przesada, co myślisz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

też mi to się rzuciło i początkowo chciałem wydzielić, ale to tak naprawdę tylko tworzenie obiektu, nie wiem czy ma sens robić oddzielny util pod to, chociaż z drugiej strony sporo linii kodu zajmuje xd, hm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zrobiłem i jakoś średnio mi podoba xd, więc chyba zostawię jak jest obecnie bez wydzielenia tego

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

@juris-97 juris-97 merged commit dc20206 into master May 20, 2022
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

3 participants