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

Cross-platform safe temp file #146

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

mccalluc
Copy link
Contributor

@mccalluc mccalluc commented Nov 5, 2024

  • Fix /tmp might not be portable #141
  • On the good side, this is more likely to work on Windows
  • On the bad side, a new temp directory is created on every run
    • We could clean it up in the session.on_ended handler, but I'm not sure the complexity is worth it?

time: ~1 hour, mostly thinking about different options.

@ekraffmiller ekraffmiller self-assigned this Nov 7, 2024
@ekraffmiller
Copy link
Member

As another option, could we use gettempdir()? It supports different environments.
Or, can we create demo.csv just once and include it in the application?

@mccalluc
Copy link
Contributor Author

mccalluc commented Nov 7, 2024

Good call!

Copy link
Member

@ekraffmiller ekraffmiller left a comment

Choose a reason for hiding this comment

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

See comment about using a temp dir

@mccalluc
Copy link
Contributor Author

mccalluc commented Nov 7, 2024

It looks like that is still doesn't return something as simple as /tmp:

>>> from tempfile import gettempdir
>>> gettempdir()
'/var/folders/jm/40wztt291610f8pk5w0sw20r0000gq/T'

I'd prefer to keep the status quo: I think it's nice to have a directory that is just for this application, rather than sharing one that could inadvertently interfere with other applications.

@mccalluc
Copy link
Contributor Author

mccalluc commented Nov 7, 2024

Second, or third thought: Would be good to try checking in the generated file. One the fence, but it would mitigate this whole temp directory problem.

@mccalluc mccalluc marked this pull request as draft November 7, 2024 20:00
@mccalluc mccalluc removed the request for review from ekraffmiller November 7, 2024 20:00
@ekraffmiller ekraffmiller removed their assignment Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending
Development

Successfully merging this pull request may close these issues.

/tmp might not be portable
2 participants