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 custom field support and add include_from_annotations. #54

Merged

Conversation

phbernardes
Copy link
Contributor

Hi,

This PR will:

  • Add support to include_from_annotations as shown in README and test_include_from_annotations.
  • Fix the error shown in test_multiple_level_relations where a custom field (user_cache in this example) is present in the Schema.

Thank you.

@jordaneremieff
Copy link
Owner

Hi @phbernardes, thanks for the PR!

I think this is a good idea, but prefer to handle the include from annotations case without introducing a new configuration flag. Currently a config error will be raised if both include and exclude are defined, but the behavior in the proposed changes would be inconsistent with that expectation. We could solve this by introducing additional validations to check when both the flag and include or exclude options are provided, but even still I think we ought to keep the amount of custom Djantic options in the config minimal.

What do you think about something like a special "__annotations__" string that could be set as the include to implement this behaviour?

For example:

class ProfileSchema(ModelSchema):
    
    website: str

    class Config:
        model = Profile
        include = "__annotations__"

@phbernardes
Copy link
Contributor Author

Hi @phbernardes, thanks for the PR!

I think this is a good idea, but prefer to handle the include from annotations case without introducing a new configuration flag. Currently a config error will be raised if both include and exclude are defined, but the behavior in the proposed changes would be inconsistent with that expectation. We could solve this by introducing additional validations to check when both the flag and include or exclude options are provided, but even still I think we ought to keep the amount of custom Djantic options in the config minimal.

What do you think about something like a special "__annotations__" string that could be set as the include to implement this behaviour?

For example:

class ProfileSchema(ModelSchema):
    
    website: str

    class Config:
        model = Profile
        include = "__annotations__"

Hi @jordaneremieff, thanks for this review. Good point, implemented it in 3a15978.

@jordaneremieff jordaneremieff merged commit 7321b54 into jordaneremieff:main Apr 10, 2022
@phbernardes
Copy link
Contributor Author

@jordaneremieff, thank you for merging this. Can you please create a release?

@jordaneremieff
Copy link
Owner

@phbernardes sure, will do this week.

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