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

Implementation for new SMT ExtractTopicFromValueSchema and tests #93

Conversation

mikaka-paf
Copy link
Contributor

Implementation for new SMT ExtractTopicFromValueSchema and tests

Copy link
Contributor

@jjaakola-aiven jjaakola-aiven 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 for the contribution @mikaka-paf.
I did a static pass of the code now, some notes and change requests commented.

Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

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

Few more notes to improve the README.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
jjaakola-aiven
jjaakola-aiven previously approved these changes Jun 8, 2023
@mikaka-paf mikaka-paf force-pushed the Initial-test-with-SMT-ExtractTopicFromEventType branch from 673f9c3 to 06d5c1d Compare June 8, 2023 08:05
@jjaakola-aiven
Copy link
Contributor

@mikaka-paf Could you still fix the typo from commit message to Implementation for new SMT extractTopicFromValueSchema and tests. Missing e in the xtractTopic....

@mikaka-paf mikaka-paf force-pushed the Initial-test-with-SMT-ExtractTopicFromEventType branch from 06d5c1d to 08c0396 Compare June 8, 2023 08:36
Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

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

Some more comments

Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

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

Some more comments

Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

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

Some more comments

Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

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

Thanks fro addressing all the comments @mikaka-paf! Looks good to me know.

@AnatolyPopov AnatolyPopov merged commit a227738 into Aiven-Open:master Jun 11, 2023
@jeqo jeqo mentioned this pull request Jun 21, 2023
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.

3 participants