Refactor collect and send algorithm #19
Conversation
| globs.tab_width = kwargs.get("tab_width", 8) | ||
| globs.context_line_count = kwargs.get("context_line_count", 200) | ||
|
|
||
| globs.attachments = kwargs.get("attachments", []) | ||
| attribute_manager.add(kwargs.get("attributes", {})) |
There was a problem hiding this comment.
instead of using kwargs, why not define the arguments directly with default values?
e.g.
def initialize(attributes={}, token=None, timeout=4)
There was a problem hiding this comment.
I will handle it in a different pull request to reduce the scope of this change
backtracepython/report.py
Outdated
| "sourceCode": add_source_code( | ||
| source_file, source_code_dict, source_path_dict, line | ||
| ), | ||
| "library": source_file, |
There was a problem hiding this comment.
maybe it would be better to use filename here only?
There was a problem hiding this comment.
The source code integration (with the source code service and source code injection in the client side) won't work
| def send(self): | ||
| if len(self.log_lines) != 0 and "Log" not in self.report["annotations"]: | ||
| self.report["annotations"]["Log"] = self.log_lines | ||
| from backtracepython.client import send_worker_report | ||
| from backtracepython.client import send |
There was a problem hiding this comment.
can we use global imports?
There was a problem hiding this comment.
unfortunately, this would require breaking the existing API. Our current interface allows the record to be sent directly via the send method. Since client has to have a reference to report (creating a report on unhandled exception or sending a message), and report has a reference to the client due to the send method, we have a circular reference
backtracepython/report_queue.py
Outdated
| self.report_queue.task_done() | ||
|
|
||
| def add(self, report, attachments): | ||
| self.report_queue.put((report, attachments)) |
There was a problem hiding this comment.
you'll want to use put_nowait here probably
There was a problem hiding this comment.
that's true. Changing it. Thanks
backtracepython/request_handler.py
Outdated
| self.debug_api( | ||
| "Submitting a payload to {},\n {}\n".format(self.submission_url, payload) | ||
| ) |
There was a problem hiding this comment.
maybe it would be a better idea to format in the debug_api method? This way you won't have to do string formatting if debugging is disabled
There was a problem hiding this comment.
On my todo list - thanks for pointing out!
backtracepython/request_handler.py
Outdated
| if not self.debug: | ||
| return | ||
|
|
||
| print(message) |
There was a problem hiding this comment.
Print to stderr. Also, maybe prefix with some kind of category?
| if not os.path.exists(attachment): | ||
| continue |
There was a problem hiding this comment.
if you do try/catch afterwards, do you need to check this here?
backtracepython/request_handler.py
Outdated
| finally: | ||
| return None |
There was a problem hiding this comment.
do you need to actually return None?
backtracepython/request_handler.py
Outdated
| response.status_code, response.text | ||
| ) | ||
| ) | ||
| return None |
There was a problem hiding this comment.
do you need to actually return None?
There was a problem hiding this comment.
oh cool - I thought I need to return a result from the method directly. Changing.
Co-authored-by: Sebastian Alex <sebapotok@gmail.com>
…ented code review feedback
No description provided.