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

fix reduce type constraints on JSON materializer #774

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented Mar 21, 2024

This issue is a follow-up on #690

Before, the JSON Loader&Saver would only support dict type objects. However, the JSON format specs are more flexible. We then introduced support for List[dict].

However, it seems reasonable to support more generally list (of str, int, None, etc.). By making it more flexible, we delegate to the underlying json Python library to surface "not json-serializable" errors to the user.

Changes

  • changed supported types from List[dict] to list
  • adjusted tests accordingly

How I tested this

  • test succeed locally

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@zilto zilto added the enhancement New feature or request label Mar 21, 2024
Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

OK, I think this should be 100% backwards compatible. Only issue is if the original json gets passed over to someone's custom json one, but that should work anyway (as we'll do the same thing). Might want to add a note on this.

Basically, we evaluate on type + order -- they're supposed to add their custom one in last, as it is done in reverse order. Given that, I don't think this will break anything.

@zilto zilto merged commit 27de197 into main Mar 21, 2024
23 checks passed
@zilto zilto deleted the fix/json-materializer branch March 21, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants