-
Notifications
You must be signed in to change notification settings - Fork 91
Refactored report format to use Pydantic #1258
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
Conversation
qkaiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first quick pass over the code, some of my colleagues will also have a look :)
|
Code looks ok, let's run the pipeline to see what fails. |
qkaiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to run pre-commit before pushing code:
pre-commit install
pre-commit run -a
You can also check typing by running pyright from the repo root.
|
I went over the changes and I am mostly happy with them, good job! The base64 process output encoding is cool, as it is more portable than python's string escapes. I disliked that we need to fight with the type checker about the discriminator values introducing a bit convoluted inheritance hierarchy. My suggestion would be to use callable discriminators eliminating the need for the I've played around with the idea on how it would look like here: garrett-zetier/unblob@1250-pydantic-report...onekey-sec:unblob:pull/1250/review |
|
I like your implementation of the report models much better. It is much cleaner. Let me know if you want me to continue with those changes on my fork. One other thing that seems to be missing is that the CI tests are failing because the |
If you are up to it, then please do so. If you say that you've done enough, that's fine as well, we can take over.
Yes, it is missing from the |
|
@vlaci I'll take a look and finish it up. Thank you guys for assisting! |
|
I believe everything should be addressed. All tests and pipelines should pass now. |
vlaci
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy about the current state of the code
|
Thanks everyone for their assistance and time reviewing! I am looking forward to these changes in a future release. |
…r deserialization.
…ons to support Python 3.9.
…ored vulture whitelisting for report.
fbacc68 to
965fa39
Compare
|
I have rebased from the latest on the upstream main branch. Is there anywhere I should document usage of the updated report with Pydantic? |
We don't publish API docs, and this change is API-only, so I don't think that this change needs extra documentation |
Resolves #1250
Overview
These changes update the JSON report format (
--reportoption) so thatpydanticmodels are used to serialize report data to JSON format rather thanattrs. This allows for easier deserialization in the future, and a Pydantic TypeAdapter is provided to make reading from the JSON files back into Python objects easier. A test has been added to test deserialization.Testing
Update the environment to the latest since
pydantichas been added as a dependency.The tests have been updated to verify the latest report format, so they can be run to verify these changes. Specifically, the test_models.py (run
uv run pytest tests/test_models.py -v) verifies both serialization and deserialization.You can run
unblobwith the--reportoption to manually verify the JSON report. For example, runuv run unblob --report report.json my.bin