-
Notifications
You must be signed in to change notification settings - Fork 10
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
util: add TempFile helper #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition. I always wondered why it's not available in the stdlib.
I like you used the Option pattern.
Please find a few suggestions about the tests that should be improved
t.t.Cleanup(f) | ||
} | ||
|
||
func TestTempFile(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wpuld like to suggest you adding tests for:
- Path() works as expected. Meaning you fi d the tempfile in the expected folder
- Path work even with not empty folder
- failure + error message if Path doesn't exist (unless you plan to create the folder if missing)
- failure + error message validation if tempfile cannot be created (permission, or server has no more space)
- existing files in existing Path folder are still present after Cleanup
- multiple options works: Mode+Path+Pattern+Content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path() works as expected.
This is in the subtest uses custom path
of file is deleted after test
. Using a subtest with a custom path was necessary for testing file deletion, so I decided not to create a redundant test for Path
. I'll update the name of the deletion test to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
}) | ||
|
||
_, err := os.Stat(path) | ||
if err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing there is no error is good, but testing it returns a file not found would be better.
data []byte | ||
mode *fs.FileMode | ||
namePattern string | ||
path *string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have used
path *string | |
path string |
Checking path is not-empty would be enough, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty path is meaningful, as it tells os.CreateTemp
to use "the default directory for temporary files, as returned by TempDir." I forgot to document that behavior. I'll leave the pointer there, fix the doc comment on Path
, and rename Path
to Dir
to match the argument in os.CreateTemp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh indeed. That's right. I faced this once. Then I forgot
Co-authored-by: ccoVeille <[email protected]>
Co-authored-by: ccoVeille <[email protected]>
Co-authored-by: ccoVeille <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Incorporated a couple suggestions from @ccoVeille.
I'll publish this as |
TempFile
and related functional settings is in a newutil
packagefixes #35