Skip to content

Commit 4728afe

Browse files
committed
(GH-26) Remove the use of Pathname
While Pathname is a great utility it is not supported cross platform > ... however non-Unix pathnames are supported experimentally. ... [1] This commit changes; * The usage of Pathname back to core Dir and File classes * Adds a helper method dir_exists? to make testing easier * Fixes the file_exist? method to check if the thing that exists is actually not a directory * Adds tests to ensure the directory traversing terminates correctly [1] http://ruby-doc.org/stdlib-2.1.9/libdoc/pathname/rdoc/Pathname.html
1 parent 8a65fb8 commit 4728afe

File tree

2 files changed

+35
-14
lines changed

2 files changed

+35
-14
lines changed

lib/puppet-languageserver/document_store.rb

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -84,25 +84,28 @@ def self.store_has_puppetfile?
8484
# root of the module/control repo actually is
8585
def self.find_root_path(path)
8686
return nil if path.nil?
87+
filepath = File.expand_path(path)
8788

88-
filepath = Pathname.new(path).expand_path
89-
return nil unless filepath.exist?
90-
91-
if filepath.directory?
89+
if dir_exist?(filepath)
9290
directory = filepath
91+
elsif file_exist?(filepath)
92+
directory = File.dirname(filepath)
9393
else
94-
directory = filepath.dirname
94+
return nil
9595
end
9696

97-
root_path = nil
98-
directory.ascend do |p|
99-
if file_exist?(p.join('metadata.json')) || file_exist?(p.join('Puppetfile'))
100-
root_path = p.to_s
101-
break
97+
until directory.nil?
98+
break if file_exist?(File.join(directory, 'metadata.json')) || file_exist?(File.join(directory, 'Puppetfile'))
99+
parent = File.dirname(directory)
100+
# If the parent is the same as the original, then we've reached the end of the path chain
101+
if parent == directory
102+
directory = nil
103+
else
104+
directory = parent
102105
end
103106
end
104107

105-
root_path
108+
directory
106109
end
107110
private_class_method :find_root_path
108111

@@ -140,8 +143,13 @@ def self.store_details
140143
private_class_method :store_details
141144

142145
def self.file_exist?(path)
143-
File.exist?(path)
146+
File.exist?(path) && !File.directory?(path)
144147
end
145148
private_class_method :file_exist?
149+
150+
def self.dir_exist?(path)
151+
Dir.exist?(path)
152+
end
153+
private_class_method :dir_exist?
146154
end
147155
end

spec/languageserver/integration/puppet-languageserver/document_store_spec.rb

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747

4848
RSpec.shared_examples 'a cached workspace' do
4949
it 'should cache the information' do
50-
expect(subject).to receive(:file_exist?).at_least(:once)
50+
expect(subject).to receive(:file_exist?).at_least(:once).and_call_original
5151
result = PuppetLanguageServer::DocumentStore.store_root_path
5252
# Subsequent calls should be cached
5353
expect(subject).to receive(:file_exist?).exactly(0).times
@@ -60,7 +60,7 @@
6060
result = PuppetLanguageServer::DocumentStore.store_root_path
6161
# Expire the cache
6262
PuppetLanguageServer::DocumentStore.expire_store_information
63-
expect(subject).to receive(:file_exist?).at_least(:once)
63+
expect(subject).to receive(:file_exist?).at_least(:once).and_call_original
6464
result = PuppetLanguageServer::DocumentStore.store_root_path
6565
# Subsequent calls should be cached
6666
expect(subject).to receive(:file_exist?).exactly(0).times
@@ -70,6 +70,15 @@
7070
end
7171
end
7272

73+
RSpec.shared_examples 'a terminating file finder' do |expected_file_calls, expected_dir_calls|
74+
it 'should only traverse until it finds an expected file' do
75+
# TODO: This test is a little fragile but can't think of a better way to prove it
76+
expect(subject).to receive(:file_exist?).exactly(expected_file_calls).times.and_call_original
77+
expect(subject).to receive(:dir_exist?).exactly(expected_dir_calls).times.and_call_original
78+
result = PuppetLanguageServer::DocumentStore.store_root_path
79+
end
80+
end
81+
7382
# Empty or missing workspace
7483
context 'given a workspace option which is nil' do
7584
let(:server_options) { {} }
@@ -121,6 +130,7 @@
121130

122131
it_should_behave_like 'a puppetfile workspace', expected_root
123132
it_should_behave_like 'a cached workspace'
133+
it_should_behave_like 'a terminating file finder', 4, 1
124134
end
125135

126136
context 'given a workspace option which has a parent directory with a puppetfile' do
@@ -135,6 +145,7 @@
135145

136146
it_should_behave_like 'a puppetfile workspace', expected_root
137147
it_should_behave_like 'a cached workspace'
148+
it_should_behave_like 'a terminating file finder', 8, 1
138149
end
139150

140151
# Module metadata style workspaces
@@ -149,6 +160,7 @@
149160

150161
it_should_behave_like 'a metadata.json workspace', expected_root
151162
it_should_behave_like 'a cached workspace'
163+
it_should_behave_like 'a terminating file finder', 3, 1
152164
end
153165

154166
context 'given a workspace option which has a parent directory with metadata.json' do
@@ -163,5 +175,6 @@
163175

164176
it_should_behave_like 'a metadata.json workspace', expected_root
165177
it_should_behave_like 'a cached workspace'
178+
it_should_behave_like 'a terminating file finder', 5, 1
166179
end
167180
end

0 commit comments

Comments
 (0)