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

Feature/traceback serialization #75

Merged

Conversation

ksamuel
Copy link
Contributor

@ksamuel ksamuel commented Jul 12, 2024

This is the first draft of what we talked about with the KISSest solution I can think of:

  • store the serialized exception in the exception dict in _result
  • add a traceback property to read it if needed

I had to make assumption about the None issue I mentioned in #68 (comment) since the branch is now rebased, force pushed and deleted. I resolved to return None from the traceback property in case of complex exceptions.

The PoC contains passing tests, including types and linting, but no documentation yet, awaiting your feedback to see if this is the spirit you are looking for.

I can squash if needed of course.

Copy link
Owner

@RealOrangeOne RealOrangeOne left a comment

Choose a reason for hiding this comment

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

Thanks so much for putting this together - it'll be a great boost for usability, without much extra maintenance burdern.

I'd also recommend rebasing your branch, now I've merged some more things in.

.vscode/settings.json Outdated Show resolved Hide resolved
tests/tests/test_utils.py Show resolved Hide resolved
django_tasks/task.py Outdated Show resolved Hide resolved
django_tasks/task.py Show resolved Hide resolved
django_tasks/task.py Outdated Show resolved Hide resolved
tests/tests/test_utils.py Outdated Show resolved Hide resolved
tests/tests/test_utils.py Outdated Show resolved Hide resolved
@ksamuel ksamuel force-pushed the feature/traceback-serialization branch from 30b7711 to cf07147 Compare July 15, 2024 13:12
@ksamuel
Copy link
Contributor Author

ksamuel commented Jul 15, 2024

Traceback are now strings, stylistic remarks are addressed, extranous file is removed. Typing is improved. Missing the C module stack trace for now.

@ksamuel
Copy link
Contributor Author

ksamuel commented Jul 15, 2024

I'll fix the linter later, meanwhile, is the PR going in the right direction?

Copy link
Owner

@RealOrangeOne RealOrangeOne left a comment

Choose a reason for hiding this comment

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

Definitely the right direction 🥳 Couple nits, but otherwise this is looking really nice!

tests/tests/test_utils.py Outdated Show resolved Hide resolved
django_tasks/task.py Show resolved Hide resolved
django_tasks/task.py Outdated Show resolved Hide resolved
@ksamuel
Copy link
Contributor Author

ksamuel commented Jul 16, 2024

Nits addressed, linting passed. Once everything matches your expectations, I'll squash and force push.

@RealOrangeOne
Copy link
Owner

Looking at the current test failures, asserting the full depth of the stacktrace is likely to cause issues, and ties the tests to how the current version of Python implements it. Given format_exception is a built-in, I'm happy to trust that it works, and just throw some assertions in for the message itself and perhaps a file path or 2.

@ksamuel
Copy link
Contributor Author

ksamuel commented Jul 16, 2024

Indeed. Especially since error messages have been the focus of improvements by Pablo Galindo Salgado. I'll make the checks more fuzzy.

@ksamuel
Copy link
Contributor Author

ksamuel commented Jul 16, 2024

Better, now we test that the data looks like we put a traceback in there, but doesn't care much about the formatting of it. I also ran the sqlite test for good measure, although I don't expect the result store to be affected in any way.

@RealOrangeOne RealOrangeOne merged commit 9cc3b60 into RealOrangeOne:master Jul 16, 2024
22 checks passed
@ksamuel ksamuel deleted the feature/traceback-serialization branch July 16, 2024 14:26
@ksamuel
Copy link
Contributor Author

ksamuel commented Jul 16, 2024

Cheers, and thanks for all the work on a very useful addition to django.

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