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

Added ability for .json() to accept kwargs. model.json() can now crea… #307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

coltonbh
Copy link
Collaborator

…te a pretty string with .json(indent=4)

Description

The current implementation of serialize does not accept generic kwargs, so we loose the basic ability to modify serialized output, like pretty printing a json string with model.json(indent=4). I'm implemented serialize to be slightly more generic so we maintain functionality described in the pydantic docs

Changelog description

Added ability for .json() to accept kwargs. model.json() can now create a pretty string with .json(indent=4)

Status

  • [x ] Code base linted
  • [ x] Ready to go

@coltonbh
Copy link
Collaborator Author

Tagging @loriab and @bennybp for a review. Thanks guys! The CI format and build are failing for reasons unrelated to my PR.

@coltonbh coltonbh self-assigned this Mar 25, 2023
@@ -127,6 +127,7 @@ def serialize(
exclude_unset: Optional[bool] = None,
exclude_defaults: Optional[bool] = None,
exclude_none: Optional[bool] = None,
**kwargs,
Copy link
Collaborator

Choose a reason for hiding this comment

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

My only hesitation is because I don't know if serialize was written w/o kwargs as a defensive measure. Certainly the indentation use to which you've put kwargs is reasonable and harmless, though. @bennybp, any complications to the database if formats can change through this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this will cause any problems on my end

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