-
Notifications
You must be signed in to change notification settings - Fork 212
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 shortening of local vars #130
Conversation
a0cace4
to
44c1126
Compare
One issue with this is that it doesn't handle dicts with lots of entries. I'm not sure how we could handle this correctly, due to insertion order not being preserved previous to Python 3.6. So we can't add a |
we used to handle it by sorting the keys and then only taking the first X number of items |
@@ -413,16 +418,21 @@ def _build_msg_for_logging(self, event_type, date=None, context=None, custom=Non | |||
log = event_data.get('log', {}) | |||
if stack and 'stacktrace' not in log: | |||
if stack is True: | |||
frames = stacks.iter_stack_frames() | |||
frames = stacks.iter_stack_frames(skip=3) |
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.
why do we need this now?
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.
It's a bug I noticed when looking at the frames in the test. It included some frames from the agent, so the first frame wasn't where you actually had the logger
call.
I suspect there are more issues with too many frames in other places we collect them, but I'll leave that for another PR.
else: | ||
ret = func(name, var) | ||
del context[objid] | ||
context.remove(objid) | ||
return ret |
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.
instead of the change on line 43 and instead of the else
clause here, could this just be:
return func(name, var)
?
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.
that would run func
on dict
as well. Which isn't entirely wrong, but shorten
doesn't have support for dicts yet. I'll try to add support for dict
in shorten
in a different PR and will change it to your suggested logic one that's in place
44c1126
to
c9048f1
Compare
This adds configuration option for shortening local variables, as well as a fix for shortening lists