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

Use CsharpNamespace when Package is empty in ConfluentProtobufTypeResolver #406

Closed
wants to merge 3 commits into from

Conversation

nazarii-piontko
Copy link

Description

This pull request is a basic implementation of the issue addressed here: #404

How Has This Been Tested?

Unit tests for ConfluentProtobufTypeResolver has been created (ConfluentProtobufTypeNameResolverTests).

Two cases are tested there, one when Package is non-empty and the second when Package is empty but CsharpNamespace is available.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests to cover my changes
  • I have made corresponding changes to the documentation

Disclaimer

By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement

Copy link
Contributor

@joelfoliveira joelfoliveira left a comment

Choose a reason for hiding this comment

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

Hi @nazarii-piontko,

First, thank you for contributing and helping KafkaFlow evolve as a project.

Although this solution does work we think that one of your suggestions in the issue description is aligned with the existing vision for KafkaFlow evolution.

I'm referring to the third solution described in the issue, registering an IAsyncSchemaRegistryTypeNameResolver which would allow overriding this behavior by the client. Nevertheless, the current implementation should work as a "backup" in case the IAsyncSchemaRegistryTypeNameResolver is not declared.

Could you change this PR for that implementation?

@nazarii-piontko
Copy link
Author

Hi @joelfoliveira, thank you for your comment. Sure, I could make my solution more generic. Let me close this PR and go ahead with updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants