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

Make temporary_path() decorator create directories #1919

Merged

Conversation

Tarrasch
Copy link
Contributor

Description

I noticed that my program crashed in the cases where I didn't manually
create the directories. And in all cases so far I've needed for the
directories to be created.

Will I definitely think this feature should be possible to turn off, I
think it's best to have as "on by default". But I left it out for anyone
making a patch in the future.

Have you tested this? If so, how?

I've added test cases and tested it very lightly in production.

I noticed that my program crashed in the cases where I didn't manually
create the directories. And in all cases so far I've needed for the
directories to be created.

Will I definetly think this feature should be possible to turn off, I
think it's best to have as "on by default". But I left it out for anyone
making a patch in the future.
@mention-bot
Copy link

@Tarrasch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jcrobak, @steenzout and @gpoulin to be potential reviewers.

*Note*: This method is optional, not all FileSystem subclasses implements it.

*Note*: parents and raise_if_exists were added in August 2014. Some
implementations might not support these flags yet.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A note for however is curious.

I think these two notes add negative value only. In general we want people to implement this method (in particular after this patch). If we leave these old (and currently mis-rendered) notes, file system implementors might not implement this method referring to this.

@Tarrasch
Copy link
Contributor Author

If nobody else complains I'll merge this tomorrow. :)

(I've verified that the Travis Issue is not this patch's fault)

@Tarrasch Tarrasch merged commit 52f0e49 into spotify:master Nov 22, 2016
@Tarrasch Tarrasch deleted the make_temporary_path_decorator_create_dirs branch November 22, 2016 08:19
This was referenced Jun 29, 2022
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.

3 participants