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

Add to_dict method and from_dict classmethod on Tracebacks #5

Merged
merged 3 commits into from
Mar 30, 2015

Conversation

beckjake
Copy link

This makes it possible to pass tblib.Traceback around as JSON/YAML, and easy to write custom JsonEncoder/JsonDecoder classes for that purpose.

@landscape-bot
Copy link

Code Health
Repository health increased by 11% when pulling 7c180c3 on beckjake:json into d3e56d1 on ionelmc:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.13%) to 91.72% when pulling 7c180c3 on beckjake:json into d3e56d1 on ionelmc:master.

@ionelmc
Copy link
Owner

ionelmc commented Mar 28, 2015

Hey, thanks for the PR. Can you also add couple tests for these new functions?

@beckjake
Copy link
Author

I actually did add a doctest in README.rst for converting to dict -> json -> dict -> Traceback. I'm not sure why coverage would have decreased, any suggestions for other tests to add?

@ionelmc
Copy link
Owner

ionelmc commented Mar 28, 2015

('f_globals', dct['tb_frame']['f_globals']),
('f_code', _AttrDict((k, v) for k, v in dct['tb_frame']['f_code'].items()))
))
frame['f_code']['co_lnotab'] = frame['f_code']['co_lnotab'].encode('latin1')
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be encoded to something else? Like hex encoding. I doubt latin1 can be used for binary data.

Also, we might not need co_lnotab at all (it just maps bytecode offsets to source linenumbers).

Copy link
Owner

Choose a reason for hiding this comment

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

I still need to test that theory about not needing co_lnotab

Copy link
Author

Choose a reason for hiding this comment

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

Oh, yeah - I suppose I don't even need that try/except, I just kept it so attr access failures looked a bit more natural.

As for choosing latin1, you could use any one-byte encoding, they're all the same for this. Latin1 is just a pretty common one.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I just found out 'hex' doesn't work, it requires that you have an even-length bytes to decode.

The tricky part about encoding co_lnotab is that you start with bytes, but json requires unicode. So you have to find an encoding that maps any arbitrary bytes to unicode code points, and then back without any changes. It would definitely be nice if it weren't required.

Copy link
Owner

Choose a reason for hiding this comment

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

I suspect it's not needed at all. Try setting it to "" (remove it from everywhere).

Copy link
Owner

Choose a reason for hiding this comment

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

Alternatively you could convert it to a list of ints, if there's no suitable encoding.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like that worked just fine.

@landscape-bot
Copy link

Code Health
Repository health increased by 11% when pulling ed04809 on beckjake:json into d3e56d1 on ionelmc:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.96%) to 92.81% when pulling ed04809 on beckjake:json into d3e56d1 on ionelmc:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.96%) to 92.81% when pulling ed04809 on beckjake:json into d3e56d1 on ionelmc:master.

@ionelmc ionelmc merged commit ed04809 into ionelmc:master Mar 30, 2015
@ionelmc
Copy link
Owner

ionelmc commented Mar 30, 2015

Alright, it's merged now and released on PyPI. Thanks!

@beckjake
Copy link
Author

beckjake commented Apr 2, 2015

Thank you! I'm looking forward to making use of this, great to have it on pypi.

@beckjake beckjake deleted the json branch April 2, 2015 01:44
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.

None yet

4 participants