Conversation
ccoVeille
left a comment
There was a problem hiding this comment.
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.
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.
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.
| }) | ||
|
|
||
| _, err := os.Stat(path) | ||
| if err == nil { |
There was a problem hiding this comment.
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.
I might have used
| path *string | |
| path string |
Checking path is not-empty would be enough, no?
There was a problem hiding this comment.
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.
Oh indeed. That's right. I faced this once. Then I forgot
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
shoenig
left a comment
There was a problem hiding this comment.
LGTM! Incorporated a couple suggestions from @ccoVeille.
|
I'll publish this as |
TempFileand related functional settings is in a newutilpackagefixes #35