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

feat: Possibility for original schema string in objects Meta class. #545

Merged

Conversation

bengels97
Copy link
Contributor

(ModelGenerator-in-generator.py,-templates.py)
Added new template and "original_schema" key check in metadata_field_templates to allow adding of original schema string in class meta class field "original_schema"

We noticed it's possible for the generated avro schema, from the generated models, to deviate from the original schema. In order to guarantee exact matching, the original schema can be included in the models themselves.

Original issue: #544

Curious to hear your thoughts on this. It's a nice workaround for our issue without breaking anything of the original functionality. I've tried to leverage the existing code base. But as you'll find, the possibility of including the original schema string is not obvious from the current code as is.

P.S: Also went ahead to fix, what seemed to be a typo, matadata_field_templates -> metadata_field_templates

…e and "original_schema" key check in metadata_field_templates to allow adding of original schema string in class meta class field "original_schema"

We noticed it's possible for the generated avro schema, from the generated models, to deviate from the original schema. In order to guarantee exact matching, the original schema can be included in the models themselves.
@marcosschroh
Copy link
Owner

marcosschroh commented Feb 16, 2024

Thanks for the PR.

Ideally, the generated model must match perfectly the original schema, unfortunately is not the case for avro types that have inner names (arrays, enums, fixed and maps). This issue is similar to your problem.

We can use your solution until we implement a new fix.

Regarding the PR I have suggestions:

  1. Include the new template by default, so end users won't have to specify all the templates
  2. Add a new option to ModelGenerator called include_original_schema with default to False. Then the end user can render the original schema:
model_generator = ModelGenerator(include_original_schema=True)
  1. Include in the docs the new feature

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1d6ef42) 99.18% compared to head (7503273) 99.65%.
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
+ Coverage   99.18%   99.65%   +0.46%     
==========================================
  Files          30       30              
  Lines        1712     1721       +9     
  Branches      307        0     -307     
==========================================
+ Hits         1698     1715      +17     
  Misses          6        6              
+ Partials        8        0       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bengels97
Copy link
Contributor Author

bengels97 commented Feb 16, 2024

Thanks for the feedback.

I have implemented your feedback and included the new feature in the docs of model_generator.

@bengels97
Copy link
Contributor Author

All pipelines are completing successfully now, with the exception of the deploy-preview though.

Seems there is an issue with the pipeline itself?
image

@marcosschroh
Copy link
Owner

All pipelines are completing successfully now, with the exception of the deploy-preview though.

Seems there is an issue with the pipeline itself? image

The github action can not handle PRs from forks.

@marcosschroh marcosschroh merged commit 9bc28b2 into marcosschroh:master Feb 16, 2024
7 of 8 checks passed
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

2 participants