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(pydantic): allow pydantic model instances as defaults (#64) #65

Merged

Conversation

dada-engineer
Copy link

@dada-engineer dada-engineer commented Mar 8, 2024

This allows pydantic BaseModels as default values.

Fixes #64

@dada-engineer dada-engineer force-pushed the fix/64-allow-pydantic-default-models branch from 96ad35e to 4725889 Compare March 8, 2024 09:36
@dada-engineer
Copy link
Author

linting fails because black and flake8 have a different opinion on how

class PyType(str):
    ...

should be formatted.

black now wants to do:

class PyType(str): ...

But flake8 then complains that there are multiple statements on one line.

@faph
Copy link
Contributor

faph commented Mar 18, 2024

So this flake8 vs black conflict. Is there a way that we can configure either of the tools to handle this OK?

@dada-engineer
Copy link
Author

So this flake8 vs black conflict. Is there a way that we can configure either of the tools to handle this OK?

I guess this comment makes sense: psf/black#4213 (comment)

TLDR;
deactivate flake8 rule for stuff that is handled by formatter.

@faph
Copy link
Contributor

faph commented Mar 18, 2024

Got you, so that should be a change in .flake8 file?

@dada-engineer
Copy link
Author

Looks like it, should I add this here?

@dada-engineer
Copy link
Author

Had some issue with mypy as well, fixed in same commit.

@faph
Copy link
Contributor

faph commented Mar 18, 2024

Love it!

Are we good to merge?

@dada-engineer
Copy link
Author

I am okay with it, but since I removed it lists of pydantic objects will not yet work. I'll open a new issue for this or a second PR later.

@faph
Copy link
Contributor

faph commented Mar 18, 2024

Understood. Many thanks for this contribution.

@faph faph merged commit d7289db into jpmorganchase:main Mar 18, 2024
4 checks passed
dada-engineer pushed a commit to dada-engineer/py-avro-schema that referenced this pull request Mar 18, 2024
dada-engineer pushed a commit to dada-engineer/py-avro-schema that referenced this pull request Mar 18, 2024
dada-engineer pushed a commit to dada-engineer/py-avro-schema that referenced this pull request Mar 18, 2024
@koenlek
Copy link

koenlek commented Oct 2, 2024

Thanks for making this fix! Unfortunately, I'm stuck on py-avro-schema v2 (and upgrading is going to be very hard I'm afraid). Any suggestions on how I could try and patch v2 or work around this issue?

I tried patching the make_default logic into v2, but it doesn't seem to work unfortunately...

Any hints would be highly appreciated 🙏

@koenlek
Copy link

koenlek commented Oct 2, 2024

NVM. Gotten it to work and posted it for others here: #64 (comment)

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.

pydantic classes as default values not serializable
3 participants