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: Somewhat Useful Exceptions for Unsupported Objects #7

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

robodair
Copy link
Contributor

@robodair robodair commented Nov 7, 2024

I hit a few edges when testing pydantic-glue out, and I've added a few exceptions with a bit more detail about what's going on.

@robodair robodair changed the title Support for Null Objects feat: Support for Null Objects Nov 7, 2024
@robodair robodair changed the title feat: Support for Null Objects feat: Somewhat Useful Exceptions for Unsupported Objects Nov 7, 2024
@svdimchenko
Copy link
Owner

I hit a few edges when testing pydantic-glue out, and I've added a few exceptions with a bit more detail about what's going on.

Hi there @robodair ! Thanks for your contribution. The code looks OK to me. Pls fix the pre-commit check and we can merge it.

@robodair robodair force-pushed the support-null-objects branch from d970ab3 to 96551dd Compare November 7, 2024 23:37
@robodair
Copy link
Contributor Author

robodair commented Nov 8, 2024

@svdimchenko I also have some changes to support specifying the glue type to use explicitly in pydantic with the Field argument:

json_schema_extra={
    "glue_type": "timestamp",
}

Would you like those proposed here or in a separate PR?

For a peculiarity about the "correct" way to store timestamps in the schema I'm using I need to store them as int, but want to use the glue timestamp type.

Update: Just pushed them here so they're somewhere for consideration

@svdimchenko
Copy link
Owner

@svdimchenko I also have some changes to support specifying the glue type to use explicitly in pydantic with the Field argument:

json_schema_extra={
    "glue_type": "timestamp",
}

Would you like those proposed here or in a separate PR?

For a peculiarity about the "correct" way to store timestamps in the schema I'm using I need to store them as int, but want to use the glue timestamp type.

Update: Just pushed them here so they're somewhere for consideration

It's definitely a nice feature to add. Please, add some reasonable example to the README.md file regarding this option.

Also I still see the pre-commit pipeline fails. Please run

pip install pre-commit
pre-commit install
pre-commit run --all-files

before committing the changes

@robodair
Copy link
Contributor Author

@svdimchenko should be ready to go now!

@robodair
Copy link
Contributor Author

Hey @svdimchenko I'd love to see this merged so I can consume a more official release, would you be able to kick off the pipeline and merge?

@svdimchenko
Copy link
Owner

Hey @svdimchenko I'd love to see this merged so I can consume a more official release, would you be able to kick off the pipeline and merge?

Hey @robodair the pre-commit check is still failed, pls fix.

@robodair robodair force-pushed the support-null-objects branch from 26578ce to 5ac0b47 Compare November 14, 2024 23:30
@robodair robodair force-pushed the support-null-objects branch from 2729b73 to 41492b1 Compare November 14, 2024 23:40
@robodair
Copy link
Contributor Author

@svdimchenko Serves me right, the node version on my work machine was too old so I skipped just the markdownlint check.

I enabled actions in my fork and have rectified the issues there.

I also noticed an issue in this repository when I did that, I got a failure on test 3.9 which didn't occur in the checks here. Turns out that the test actions on main in this repo all ran on Python 3.12: https://github.com/svdimchenko/pydantic-glue/actions/runs/11770931878/job/32982167948 which probably isn't want you want.

Looked into it, was a cache key issue, fixed by assigning an id to the python-version step, which I've added to this PR.

🤞 finally good to merge now I think

Copy link
Owner

@svdimchenko svdimchenko left a comment

Choose a reason for hiding this comment

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

thanks a lot for you contribution @robodair ! I'll merge and release the new version 💯

@svdimchenko svdimchenko merged commit 52d50fa into svdimchenko:main Nov 15, 2024
6 checks passed
@robodair robodair deleted the support-null-objects branch November 22, 2024 02:04
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