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

[improve][client] Add schema typing to consumer and producer #278

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

romainbrancourt
Copy link
Contributor

@romainbrancourt romainbrancourt commented Jan 3, 2023

Modifications

Add schema typing to consumer and producer

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

Could you add some schema tests to verify this PR?

@romainbrancourt
Copy link
Contributor Author

@RobertIndie
I added a test, do you think it's enough or you want me to create a test for each schemaType ?

index.d.ts Outdated
@@ -236,3 +245,21 @@ export type ConsumerCryptoFailureAction =
'FAIL' |
'DISCARD' |
'CONSUME';

export type SchemaType =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export type SchemaType =
export type SchemaType =

@k2la
Copy link
Contributor

k2la commented Jan 23, 2023

@romainbrancourt
I think this can be better if you add tests about consumer.

@romainbrancourt romainbrancourt requested review from RobertIndie and k2la and removed request for RobertIndie January 23, 2023 10:40
@k2la
Copy link
Contributor

k2la commented Jan 24, 2023

LGTM

@k2la k2la merged commit ce9b937 into apache:master Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants