-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
Improve autoreload of external packages #6459
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6459 +/- ##
===========================================
- Coverage 82.13% 71.45% -10.68%
===========================================
Files 312 314 +2
Lines 46122 46309 +187
===========================================
- Hits 37881 33091 -4790
- Misses 8241 13218 +4977
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7fd9f82
to
4d0cbe6
Compare
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 have looked at the code. I still want to test it out, but I left some comments.
I don't think we should use Pathlib for this low-level stuff. There is a time penalty for creating each of the objects. I have tried to update some, but please check.
pass | ||
self._servers = {} | ||
if server_id in self._threads: | ||
self._threads[server_id].stop() |
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.
Is it ok to raise an error now for 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.
Not quite following.
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.
The lines above, was wrapped in try/except.
for thread in self._threads.values():
try:
thread.stop()
except Exception:
pass
Thanks for the review, I'll wait for tests to pass and then apply your suggestions. |
I just did (light) some testing of the PR, and it seems to work as promised 👍 |
Co-authored-by: Simon Høxbro Hansen <simon.hansen@me.com>
Co-authored-by: Simon Høxbro Hansen <simon.hansen@me.com>
Co-authored-by: Simon Høxbro Hansen <simon.hansen@me.com>
Currently
--autoreload
only reloads the app itself and any one module that has changed. In practice this isn't really enough because most modules will be interconnected pretty deeply, so instead of reloading the specific module we are talking about we will reload all modules that were recorded during startup.