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 report serialization #215

Merged
merged 2 commits into from
Dec 12, 2023
Merged

fix report serialization #215

merged 2 commits into from
Dec 12, 2023

Conversation

zargot
Copy link
Collaborator

@zargot zargot commented Dec 7, 2023

The TableInfo class which appears in the validation report, had to be made into a TypedDict to support normal json serialization in tools using it (namely the web validation tool).

@zargot zargot requested a review from yulric December 7, 2023 17:00
@zargot zargot changed the title use typeddict for report tableinfo remove nested class types from validation report Dec 11, 2023
@zargot zargot mentioned this pull request Dec 11, 2023
Copy link
Collaborator

@yulric yulric left a comment

Choose a reason for hiding this comment

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

Can you update the commit message with the following information:

  1. Why we removed the class (because of the web tool)
  2. Why we can't type the TableInfo as a TypedDict instead of a class

In general, we should think about our usage of the jsons library. I think its a big issue if we can't use TypedDict in our code since we will eventually typing to this library.

src/odm_validation/reports.py Outdated Show resolved Hide resolved
@zargot zargot changed the title remove nested class types from validation report fix report serialization Dec 12, 2023
@zargot
Copy link
Collaborator Author

zargot commented Dec 12, 2023

@yulric, I managed to make TypedDict work, and removed "jsons".

Copy link
Collaborator

@yulric yulric 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. I wonder if we can replace the TableInfo class with a TypedDict, seems more light weight.

to get rid of jsons dependency, which doesn't play along well with
TypedDict (which we'll be using more and more).

This change also improves the code in general.
for json serialization.

Dataclass decorator is removed since it doesn't work with typeddict in
python < 3.11.
@zargot zargot merged commit 9863f9a into dev Dec 12, 2023
2 checks passed
@zargot zargot deleted the report-serial branch December 12, 2023 19:46
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