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

util: add TempFile helper #168

Merged
merged 5 commits into from
Aug 27, 2024
Merged

util: add TempFile helper #168

merged 5 commits into from
Aug 27, 2024

Conversation

brandondyck
Copy link
Contributor

  • Adds TempFile and related functional settings is in a new util package
  • Replaces ad hoc temp file helpers in file/dir helper tests
  • Bumps version to 1.10.0

fixes #35

Copy link
Contributor

@ccoVeille ccoVeille left a 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

util/tempfile_test.go Outdated Show resolved Hide resolved
t.t.Cleanup(f)
}

func TestTempFile(t *testing.T) {
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

util/tempfile_test.go Outdated Show resolved Hide resolved
})

_, err := os.Stat(path)
if err == nil {
Copy link
Contributor

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.

util/tempfile.go Outdated Show resolved Hide resolved
data []byte
mode *fs.FileMode
namePattern string
path *string
Copy link
Contributor

Choose a reason for hiding this comment

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

I might have used

Suggested change
path *string
path string

Checking path is not-empty would be enough, no?

Copy link
Contributor Author

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.

Copy link
Contributor

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

shoenig and others added 3 commits August 27, 2024 10:38
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>
Copy link
Owner

@shoenig shoenig left a 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.

@shoenig
Copy link
Owner

shoenig commented Aug 27, 2024

I'll publish this as v1.10.0-alpha.1 for a week or so to give me a chance to test drive this in a real project for a bit - just in case we want to change the public API at all.

@shoenig shoenig merged commit d605eb2 into shoenig:main Aug 27, 2024
3 checks passed
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.

idea: helper for auto cleanup of tempfile
3 participants