Skip to content
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

[stdlib] Temporary directory #2743

Open
wants to merge 2 commits into
base: nightly
Choose a base branch
from

Conversation

artemiogr97
Copy link
Contributor

@artemiogr97 artemiogr97 commented May 18, 2024

Implementation of TemporaryDirectory, a wrapper around mkdtemp that can be used as a context nanager.

@artemiogr97 artemiogr97 requested a review from a team as a code owner May 18, 2024 19:26
@artemiogr97 artemiogr97 requested a review from a team as a code owner May 18, 2024 19:33
@artemiogr97
Copy link
Contributor Author

Part of the implementation for #2018

@laszlokindrat laszlokindrat self-assigned this May 20, 2024
@artemiogr97 artemiogr97 force-pushed the TemporaryDirectory branch 2 times, most recently from 301021a to 1ceff7e Compare May 23, 2024 21:06
@laszlokindrat
Copy link
Contributor

#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?

stdlib/src/tempfile/tempfile.mojo Outdated Show resolved Hide resolved
stdlib/src/tempfile/tempfile.mojo Outdated Show resolved Hide resolved
stdlib/src/tempfile/tempfile.mojo Show resolved Hide resolved
stdlib/src/tempfile/tempfile.mojo Outdated Show resolved Hide resolved
@JoeLoser
Copy link
Collaborator

#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?

+1 to rebasing and adding a nice PR description!

Comment on lines +214 to +215
suffix: String = "",
prefix: String = "tmp",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Python these can also be None. Can we also use Optional[String] = None for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"



# TODO use shutil.rmtree (or equivalent) when it exists
fn _rmtree(path: String, ignore_errors: Bool = False) raises:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use PathLike for the path argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ksandvik
Copy link

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>
Signed-off-by: Artemio Garza Reyna <artemiogr97@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants