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

Convert from BaseModel to jsonschema + TypedDict #343

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

evalott100
Copy link
Contributor

Closes #337

@evalott100 evalott100 changed the title Convert from BaseModel to jsonschema + TypedDict` Convert from BaseModel to jsonschema + TypedDict Jan 10, 2025
@evalott100 evalott100 force-pushed the 337-add-basemodel-support-1 branch 5 times, most recently from 88e3c04 to 176413f Compare January 10, 2025 11:18
@evalott100 evalott100 requested a review from coretl January 10, 2025 11:18
@evalott100 evalott100 self-assigned this Jan 10, 2025
@evalott100
Copy link
Contributor Author

Will probably streamline the system of adding extra json schema to BaseModels too

@evalott100 evalott100 force-pushed the 337-add-basemodel-support-1 branch from 176413f to 1e860ad Compare January 10, 2025 11:29
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

I'm happy with this, and it didn't require any code changes so I think that means the checking for nulls is OK.

@danielballan please can you review the schema changes on projections?

Just to make sure, please can you point bluesky to this and run mypy src/bluesky, then point ophyd-async at this and run pyright src/ophyd_async and check it doesn't think we need to check for nulls anywhere?

@coretl
Copy link
Contributor

coretl commented Jan 10, 2025

Also, why is the test failing?

@evalott100 evalott100 marked this pull request as ready for review January 13, 2025 08:02
@evalott100
Copy link
Contributor Author

evalott100 commented Jan 13, 2025

Also, why is the test failing?

There wasn't consistency in the order of the generated __init__.py so the test for consistency in regeneration failed. Sorted them now.

I'll wait for a review and then add an ADR and update the docs within this PR once we've decided.

@evalott100 evalott100 force-pushed the 337-add-basemodel-support-1 branch 6 times, most recently from a683dc5 to 2195892 Compare January 13, 2025 10:44
@evalott100
Copy link
Contributor Author

I tested that it's working against bluesky and ophyd-async, both tests and type checking.

I had to write some custom code to include Partial documents within the jsonschema itself:

@evalott100 evalott100 force-pushed the 337-add-basemodel-support-1 branch from 2195892 to 895f19d Compare January 13, 2025 10:54
Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

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

Looks good.

Incidentally I like the new usage, too. Good use of python -m ....

@evalott100
Copy link
Contributor Author

Was going to add tests for the BaseModel themselves. I think it's better to do it in a different PR though.

@evalott100 evalott100 merged commit a8d4575 into main Jan 15, 2025
19 checks passed
@evalott100 evalott100 deleted the 337-add-basemodel-support-1 branch January 15, 2025 09:48
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.

BaseModel documents and change to schema generation
3 participants