-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Use Tempfile.create
#31099
Use Tempfile.create
#31099
Conversation
Instead of `Dir::Tmpname.make_tmpname`, an internal method which does not guarantee uniqueness, use `Tempfile.create`.
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
Previously |
@@ -98,7 +98,7 @@ def setup | |||
|
|||
@old_logger = ActionController::Base.logger | |||
|
|||
@cache_path = File.join Dir.tmpdir, Dir::Tmpname.make_tmpname("tmp", "cache") | |||
@cache_path = Tempfile.create(%w"tmp cache", Dir.tmpdir) |
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.
Seems should be here:
@cache_path = Tempfile.create(%w"tmp cache", Dir.tmpdir).path
FileUtils.rm(@cache_path)
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.
Thank you for the info.
Then it should use Dir.mktmpdir
.
As `@cache_path` is expected to be a directory name, use `Dir.mktmpdir`. And omit unnecessary `Dir.tmpdir`.
Thanks nobu. Merged |
@rafaelfranca Would you backport this change to |
Thanks for the backport. |
Instead of
Dir::Tmpname.make_tmpname
, an internal method which does not guarantee uniqueness, useTempfile.create
.Summary
Provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Other Information
If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.
If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.
Finally, if your pull request affects documentation or any non-code
changes, guidelines for those changes are available
here
Thanks for contributing to Rails!