-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Dbg r reload #983
Dbg r reload #983
Conversation
dash/testing/application_runners.py
Outdated
|
||
except (OSError, ValueError): | ||
logger.exception("process server has encountered an error") | ||
self.started = False | ||
return | ||
|
||
self.started = True | ||
|
||
return app |
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.
@rpkyle in case you need to get the tmp path, start()
returns it
) | ||
self._tmp_app_path = os.path.join( | ||
"/tmp" if not self.is_windows else os.getenv("TEMP"), | ||
uuid.uuid4().hex, |
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.
as @rpkyle mentioned, the hot reloading on R side will actively monitor all the folder under the main app file, in this case, the old approach using /tmp/app_{}.R is not safe
os.path.join(cwd, _) | ||
for _ in os.listdir(cwd) | ||
if not _.startswith("__") | ||
and os.path.isdir(os.path.join(cwd, _)) |
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 _
? I prefer to use that only when the var isn't going to be used at all.
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 also like to use that in list comprehension especially when it gets more complicated.
- shorter
- I think it's easier to read once you used to the convention, and can be more focus on the operation and logic side.
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.
Fair enough. I'd tend to use a 1-character name for that but I see your point.
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.
Fair enough. I'd tend to use a 1-character name for that but I see your point.
1-char name is anti-pylint
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.
_
isn't a 1-char name? 🤔
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.
btw, it's a valid 1-char name, certified.
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.
Just one style comment - aside from that, if @rpkyle can confirm this works as desired, I'm happy! 💃
not only confirmed by Ryan. he also promises a nice shot of espresso tomorrow. |
this PR solves the problem when dashR app is defined in a string chunk, and the app needs to work with assets or clientside js file.