Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 a context manager object and a method to call said object #357
base: master
Are you sure you want to change the base?
Add a context manager object and a method to call said object #357
Changes from 25 commits
8d1ae15
529f0a4
5bac851
4eeed1a
4714ba4
021661d
2401f78
00e4967
9c51dcc
642cc20
ea80652
83b77b5
9e4c34a
5b63fc0
0483523
ae692bf
fa6cf69
e7c0b5c
8d89888
9ca6df6
48eaa57
e5fb916
5bae76e
7378835
69b035e
430fd29
5eb2da2
00c89ec
25045f9
bcece26
1153529
d3894db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What
_flatten_nested_lists()
is doing? isn't_tags
already a flatten list and not nested?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.
no,
_tags.value
at this point would look something likewhere each inner list keeps the tags used to represent a feature flag together. I thought this was necessary because when an exception occurs in the scope of a context manager and we pop and attach the tags to the exception.
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.
Ok I see. I think we need to change the tag manager so it appends each tag to the list:
That
self.tag
should beself.tags
and we shouldn't doappend()
but maybeextend()
so_tags
is always flattenThere 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'm not sure I agree with using
extend
or keeping_tags
always flatten. In the__exit__
method:we pop from
_tags
whenever we exit the context manager. If this was flattened, we'd have to keep track of the number of times we should pop.Well, I guess this is something we can do. Would something like this be preferred?
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.
Oh ok, I see what you mean. You are right although maybe we can:
_tags
to something different that represents better that they are "context tags" or similar_LocalTags
that gives you the flatten tags. That way_tags
will hide the internal structure and I think the code that uses it can be cleaner since will not deal with flattening the list, etc...What you think?
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.
Oooo I like that! Let me see if I cam implement it :D
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.
done
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 or when would be the case where
_LocalTags
is initialized, but does not have atags
attribute inself._registry
?removing this if condition, fails this test https://github.com/rollbar/pyrollbar/blob/master/rollbar/test/test_rollbar.py#L289
with this:
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 think you should try to get the root exception that is triggering that report and see at which moment that's happening
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.
Ah. Ok ok. I think I see what is happening right now. For this test
test_report_exception_with_same_exception_as_cause
, we are running thereport_exc_info
inside athreading.Thread
and when we do that,tags
is not set in thatthread._local
, thus this runtime error when trying to callreport_exc_info
. I think I might know a way around this.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've resolved this by initializing
self._registry.tags
in theappend
andvalue
method of_LocalTags
.