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(schema): allow set _ignore_default_error to True #416

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

qfritz
Copy link

@qfritz qfritz commented Jan 21, 2025

_ignore_default_error will ignore errors like:

"Default value must match first schema in union with type: null"

...which I agree are errors but are painful to fix when backporting stuff.

WDYT?

@qfritz
Copy link
Author

qfritz commented Jan 23, 2025

Ah, deploy preview CI step is broken 🤔

@marcosschroh
Copy link
Owner

_ignore_default_error will ignore errors like:

"Default value must match first schema in union with type: null"

...which I agree are errors but are painful to fix when backporting stuff.

WDYT?

Thanks for the PR. I can be a good idea but we need to fix this in a different way. In the AvroSchema.init method you can get the extra fastavro property _ignore_default_error and use it when creating the schema: avro_schema = schema.AvroSchema(deployment_schema). The default value should be False to avoid a breaking change.

The deploy-preview step fails due the github actions because it still does not support deployments from a fork

Copy link
Owner

@marcosschroh marcosschroh left a comment

Choose a reason for hiding this comment

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

Use the property in the AvroSchema.init _ignore_default_error and use it when creating the schema.

@qfritz qfritz force-pushed the patch-1 branch 2 times, most recently from c44ae11 to 4d19c3f Compare January 27, 2025 08:15
@qfritz
Copy link
Author

qfritz commented Jan 27, 2025

Hi again, thanks for reviewing this PR.
I tried to apply your suggestion and amended my commit (sorry, my python is rusty, i hope I got it right).

@qfritz qfritz force-pushed the patch-1 branch 2 times, most recently from 087a8a3 to bcd4e4a Compare January 27, 2025 08:18
@qfritz qfritz changed the title feat(schema): set _ignore_default_error to True feat(schema): allow set _ignore_default_error to True Jan 27, 2025
@qfritz
Copy link
Author

qfritz commented Jan 27, 2025

Added tests! 🎉
I made sure that they pass. I'm not sure about all the init changes but if i didn't update both BaseSchema and AvroSchema, tests were failing.

@qfritz qfritz requested a review from marcosschroh January 27, 2025 18:12
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.

2 participants