[stdlib] Temporary directory #2743
Conversation
artemiogr97
force-pushed
the
TemporaryDirectory
branch
from
1caebd2
to
786c0a0
Compare
|
Part of the implementation for #2018 |
artemiogr97
force-pushed
the
TemporaryDirectory
branch
2 times, most recently
from
301021a
to
1ceff7e
Compare
|
#2742 has landed internally and will be included in the next nightly. Could you please rebase after that? Also, could you please update the PR description with a summary of the contents of this patch? |
artemiogr97
force-pushed
the
TemporaryDirectory
branch
from
1ceff7e
to
bcfa906
Compare
+1 to rebasing and adding a nice PR description! |
| suffix: String = "", | ||
| prefix: String = "tmp", |
There was a problem hiding this comment.
In Python these can also be None. Can we also use Optional[String] = None for these?
There was a problem hiding this comment.
Optional[String] does not play along nicely with StringLiteral, that's why I used default values for these, it should be possible to allow to use None but I think we will need 4 overloads to cover all the cases:
fn __init__(inout self, suffix: String = "", prefix: String = "tmp", ...)
fn __init__(inout self, suffix: Optional[String] = None, prefix: String = "tmp", ...)
fn __init__(inout self, suffix: String = "", prefix: Optional[String] = None, ...)
fn __init__(inout self, suffix: Optional[String] = None, prefix: Optional[String] = None, ...)
note: mkdtemp has the same "issue"
There was a problem hiding this comment.
Yeah, this is a known issue, but workaround with the overloads can be simpler:
fn __init__(inout self, suffix: String, prefix: String = "tmp", ...)
fn __init__(inout self, suffix: NoneType = None, prefix: String = "tmp", ...)
fn __init__(inout self, suffix: String = "", prefix: NoneType = None, ...)
fn __init__(inout self, suffix: NoneType = None, prefix: NoneType = None, ...)There was a problem hiding this comment.
Could you please address this in a followup?
|
|
||
|
|
||
| # TODO use shutil.rmtree (or equivalent) when it exists | ||
| fn _rmtree(path: String, ignore_errors: Bool = False) raises: |
There was a problem hiding this comment.
Can we use PathLike for the path argument?
There was a problem hiding this comment.
listdir returns a list of strings and os.path.join only takes String for now, so found it easier to use a String for now
There was a problem hiding this comment.
Okay, thanks! Would it be possible to fix this in a followup?
|
Yes, we could avoid Strings for path definitions, but rather PathLike traits, this for cross-platform support. |
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
artemiogr97
force-pushed
the
TemporaryDirectory
branch
from
bcfa906
to
92369fa
Compare
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
|
!sync |
modularbot
added
the
imported-internally
|
✅🟣 This contribution has been merged 🟣✅ Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours. We use Copybara to merge external contributions, click here to learn more. |
modularbot
added
the
merged-internally
Implementation of
TemporaryDirectory, a wrapper aroundmkdtempthat can be used as a context nanager.