Use .knapsack_pro directory for temporary files instead of the tmp directory in the user's project directory
#155
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,6 @@ class Env | |
| 'info' => ::Logger::INFO, | ||
| 'debug' => ::Logger::DEBUG, | ||
| } | ||
|
|
||
| class << self | ||
| def ci_node_total | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| module KnapsackPro | ||
| module Config | ||
| class TempFiles | ||
| def self.temp_directory_path | ||
|
There was a problem hiding this comment. Have you considered separating the ensuring the temp directory exists from returning the path? Or maybe memoizing the result or sth? Do you think there is a way to do this check once at some point, or would it always be succeptible to errors? The reason I'm wondering about this is that it seems that we are using this There was a problem hiding this comment. I was thinking about it and I decided that checking if file exists on the disk is good enough. We already do some IO operations in queue mode like saving reports and it shouldn't have significant impact on performance. If I add memorization to class method then this causes random tests failures depending on the order of tests executed because class method memorization is preserved for each call of class method. For instance I could do: def self.temp_directory_path
@temp_directory_path ||=
begin
temp_files = new
temp_files.ensure_temp_directory_exists
temp_files.temp_directory_path
end
endand then when I run tests I get randomly different tests failures: I could disable memorization with custom argument |
||
| temp_files = new | ||
| temp_files.ensure_temp_directory_exists | ||
| temp_files.temp_directory_path | ||
| end | ||
|
|
||
| def ensure_temp_directory_exists | ||
|
There was a problem hiding this comment. Do we need to keep this method and the There was a problem hiding this comment. When I move code to private
def ensure_temp_directory_exists
unless File.exists?(gitignore_file_path)
create_temp_directory
create_gitignore_file
end
end
# relative to the directory where you run knapsack_pro gem (user's project)
def temp_directory_path
'.knapsack_pro'
endthen I get error: When I move code to We call the following methods in class method temp_files.ensure_temp_directory_exists
temp_files.temp_directory_pathThere was a problem hiding this comment. Yes, this is because we are trying to access the private method from outside the object. If we are adamant about the public method remaining a class method, we could change all the other methods to be private class methods. |
||
| unless File.exists?(gitignore_file_path) | ||
|
There was a problem hiding this comment. This method is deprecated in ruby 2.7.1, and doesn't exist in ruby 3.0.2. There is an #exist? method, but I'm not sure if they are effectively the same. I was dealing with ensuring a directory exists in my past side-project. I looked up the code and noticed I was using the In case it's helpful to you, here is how I handled a similar case back then: create_needed_dirs!
def create_needed_dirs!
create!('foo')
create!('bar')
end
def create!(directory)
return false if Dir.exist?(directory)
Dir.mkdir(directory) && true
endThere was a problem hiding this comment. Good catch. I've used |
||
| create_temp_directory | ||
| create_gitignore_file | ||
| end | ||
| end | ||
|
|
||
| # relative to the directory where you run knapsack_pro gem (user's project) | ||
| def temp_directory_path | ||
| '.knapsack_pro' | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def create_temp_directory | ||
| FileUtils.mkdir_p(temp_directory_path) | ||
| end | ||
|
|
||
| def gitignore_file_path | ||
| File.join(temp_directory_path, '.gitignore') | ||
|
There was a problem hiding this comment. Do I understand correctly, that we'll create our own Does it mean that there would now need to be the There was a problem hiding this comment.
Yes.
No. Because we also ignore
There was a problem hiding this comment. Cool, I hoped it works the way you described it. Great to know it does indeed! 💚 |
||
| end | ||
|
|
||
| def gitignore_file_content | ||
| "# This directory is used by knapsack_pro gem for temporary files during tests runtime.\n" << | ||
|
There was a problem hiding this comment. I think that "for storing temporary files" sounds more proper. |
||
| "# Ignore all files, and do not commit this directory into your repository.\n" << | ||
| "# Learn more at https://knapsackpro.com\n" << | ||
| "*" | ||
| end | ||
|
|
||
| def create_gitignore_file | ||
| File.open(gitignore_file_path, 'w+') do |f| | ||
| f.write(gitignore_file_content) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| describe KnapsackPro::Config::TempFiles, :clear_tmp do | ||
| describe '.temp_directory_path' do | ||
| let(:gitignore_file_path) { '.knapsack_pro/.gitignore' } | ||
|
|
||
| subject { described_class.temp_directory_path } | ||
|
|
||
| it 'returns temporary directory path' do | ||
| expect(subject).to eq '.knapsack_pro' | ||
| end | ||
|
|
||
| it 'creates .gitignore file' do | ||
| expect(File.exists?(gitignore_file_path)).to be false | ||
|
There was a problem hiding this comment. FYI there is a nice DSL for expectations like that https://relishapp.com/rspec/rspec-expectations/v/2-0/docs/matchers/expect-change e.g. (just wanted to share this in case you forgot about it, the current one is cool by all means) |
||
| subject | ||
| expect(File.exists?(gitignore_file_path)).to be true | ||
| end | ||
|
|
||
| it '.gitignore file has content' do | ||
|
There was a problem hiding this comment. Do I understand correctly that the most important line in the file's content is the one with the asterisk ( |
||
| subject | ||
| expect(File.read(gitignore_file_path)).to include '# This directory is used by knapsack_pro gem for temporary files during tests runtime' | ||
| end | ||
| end | ||
| end | ||
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 think this should read
user's project. I would also change "folder" to "directory" (despite the duplication).