Use custom context manager for temporary file in cli.py #489
Conversation
lib/streamlit/cli.py
Outdated
There was a problem hiding this comment.
This is getting quite big now... can you pull it into a utility function?
There was a problem hiding this comment.
Yeah, I am going to change this.
|
I change the solution quite a bit. Will change title and description for this PR. |
lib/streamlit/cli.py
Outdated
| from streamlit.temporary_directory import TemporaryDirectory | ||
|
|
||
| with TemporaryDirectory() as temp_dir: | ||
| script_path = os.path.join(temp_dir, os.path.basename(file_or_url)) |
There was a problem hiding this comment.
Don't use basename with URLs. It's meant to be used with filesystem paths, so on Windows it will use backslashes. Also, it doesn't work when the URL ends in a slash.
Use something like urllib.parse.urlparse() instead.
There was a problem hiding this comment.
Also, to make sure this doesn't break in the future, can you add a test for main_run?
If tests don't work with click, you can always pull the entire body of main_run into another function called _main_run_real and test that:
@stuff
@stuff
def main_run(...):
_main_run_real(...)
There was a problem hiding this comment.
We have tests for this. I definitely had to update them :-)
lib/streamlit/cli.py
Outdated
| with TemporaryDirectory() as temp_dir: | ||
| from urllib.parse import urlparse | ||
| path = urlparse(file_or_url).path | ||
| script_path = os.path.join(temp_dir, path.rsplit('/', 1)[-1]) |
There was a problem hiding this comment.
Not sure there is a better way of doing this. This way seems "StackOverflow" accepted: https://stackoverflow.com/questions/7253803/how-to-get-everything-after-last-slash-in-a-url
There was a problem hiding this comment.
You should do path.strip('/') first to remove the final slash, if any.
|
Updated after @tvst 's comments. |
lib/streamlit/cli.py
Outdated
| with TemporaryDirectory() as temp_dir: | ||
| from urllib.parse import urlparse | ||
| path = urlparse(file_or_url).path | ||
| script_path = os.path.join(temp_dir, path.rsplit('/', 1)[-1]) |
There was a problem hiding this comment.
You should do path.strip('/') first to remove the final slash, if any.
* develop: Use custom context manager for temporary file in cli.py (streamlit#489) Update pull_request_template.md Update pull_request_template.md Handle lack of trailing slash (streamlit#528) Set auto_envvar_prefix to STREAMLIT (streamlit#477) Strip slashes in path components (streamlit#523) Create doc_improvement.md ESlint: allow @ts-ignore (streamlit#499) Update Pipfile (streamlit#505) Release 0.49.0 (streamlit#509) Removing package json (streamlit#501) Feature/input number (streamlit#416) ESLint warnings are fatal in Circle (streamlit#492) Doc updates (streamlit#286)
Issue: #301
Description: In Windows, we cannot rely on NamedTemporaryFile removing the temporary file that we use for the temporary python script on running from url. This PR adds a custom
TemporaryDirectorycontext manager to use with remote files.The
TemporaryDirectorycontext manager remove the content of the temporary directory when exits.Contribution License Agreement
By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.