From 87fa677e4e6622bc80d1f4b05edf63b4313e21c6 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Tue, 17 Sep 2019 20:21:26 +0800 Subject: [PATCH] (GH-177) Add auto-align hash rocket feature Previously the onTypeFormatting provider was created and responds to client settings, turning it on and off appropriately. This commit actually adds the on type formatter: * Only for puppet manifests (.pp) * Uses puppet-lint lexer to tokenise the document and uses the tokens to determine indenting * Will not operate if the document is over 4KB in size or the client is using tabs instead of spaces * Adds tests for the formatting --- .../manifest/format_on_type_provider.rb | 137 ++++++++++++++++++ lib/puppet-languageserver/message_router.rb | 26 +++- lib/puppet-languageserver/providers.rb | 1 + .../manifest/format_on_type_provider_spec.rb | 123 ++++++++++++++++ .../message_router_spec.rb | 73 +++++++++- 5 files changed, 356 insertions(+), 4 deletions(-) create mode 100644 lib/puppet-languageserver/manifest/format_on_type_provider.rb create mode 100644 spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb diff --git a/lib/puppet-languageserver/manifest/format_on_type_provider.rb b/lib/puppet-languageserver/manifest/format_on_type_provider.rb new file mode 100644 index 00000000..0092e24d --- /dev/null +++ b/lib/puppet-languageserver/manifest/format_on_type_provider.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +require 'puppet-lint' + +module PuppetLanguageServer + module Manifest + class FormatOnTypeProvider + class << self + def instance + @instance ||= new + end + end + + def format(content, line, char, trigger_character, formatting_options) + result = [] + # Abort if the user has pressed something other than `>` + return result unless trigger_character == '>' + # Abort if the formatting is tab based. Can't do that yet + return result unless formatting_options['insertSpaces'] == true + # Abort if content is too big + return result if content.length > 4096 + + lexer = PuppetLint::Lexer.new + tokens = lexer.tokenise(content) + + # Find where in the manifest the cursor is + cursor_token = find_token_by_location(tokens, line, char) + return result if cursor_token.nil? + # The cursor should be at the end of a hashrocket, otherwise exit + return result unless cursor_token.type == :FARROW + + # Find the start of the hash with respect to the cursor + start_brace = cursor_token.prev_token_of(:LBRACE, skip_blocks: true) + # Find the end of the hash with respect to the cursor + end_brace = cursor_token.next_token_of(:RBRACE, skip_blocks: true) + + # The line count between the start and end brace needs to be at least 2 lines. Otherwise there's nothing to align to + return result if end_brace.nil? || start_brace.nil? || end_brace.line - start_brace.line <= 2 + + # Find all hashrockets '=>' between the hash braces, ignoring nested hashes + farrows = [] + farrow_token = start_brace + lines = [] + loop do + farrow_token = farrow_token.next_token_of(:FARROW, skip_blocks: true) + # if there are no more hashrockets, or we've gone past the end_brace, we can exit the loop + break if farrow_token.nil? || farrow_token.line > end_brace.line + # if there's a hashrocket AFTER the closing brace (why?) then we can also exit the loop + break if farrow_token.line == end_brace.line && farrow_token.character > end_brace.character + # Check for multiple hashrockets on the same line. If we find some, then we can't do any automated indentation + return result if lines.include?(farrow_token.line) + lines << farrow_token.line + farrows << { token: farrow_token } + end + + # Now we have a list of farrows, time for figure out the indentation marks + farrows.each do |item| + item.merge!(calculate_indentation_info(item[:token])) + end + + # Now we have the list of indentations we can find the biggest + max_indent = -1 + farrows.each do |info| + max_indent = info[:indent] if info[:indent] > max_indent + end + # No valid indentations found + return result if max_indent == -1 + + # Now we have the indent size, generate all of the required TextEdits + farrows.each do |info| + # Ignore invalid hashrockets + next if info[:indent] == -1 + end_name_token = info[:name_token].column + info[:name_token].to_manifest.length + begin_farrow_token = info[:token].column + new_whitespace = max_indent - end_name_token + # If the whitespace is already what we want, then ignore it. + next if begin_farrow_token - end_name_token == new_whitespace + + # Create the TextEdit + result << LSP::TextEdit.new.from_h!( + 'newText' => ' ' * new_whitespace, + 'range' => LSP.create_range(info[:token].line - 1, end_name_token - 1, info[:token].line - 1, begin_farrow_token - 1) + ) + end + result + end + + private + + VALID_TOKEN_TYPES = %i[NAME STRING SSTRING].freeze + + def find_token_by_location(tokens, line, character) + return nil if tokens.empty? + # Puppet Lint uses base 1, but LSP is base 0, so adjust accordingly + cursor_line = line + 1 + cursor_column = character + 1 + idx = -1 + while idx < tokens.count + idx += 1 + # if the token is on previous lines keep looking... + next if tokens[idx].line < cursor_line + # return nil if we skipped over the line we need + return nil if tokens[idx].line > cursor_line + # return nil if we skipped over the character position we need + return nil if tokens[idx].column > cursor_column + # return the token if it starts on the cursor column we are interested in + return tokens[idx] if tokens[idx].column == cursor_column + end_column = tokens[idx].column + tokens[idx].to_manifest.length + # return the token it the cursor column is within the token string + return tokens[idx] if cursor_column <= end_column + # otherwise, keep on searching + end + nil + end + + def calculate_indentation_info(farrow_token) + result = { indent: -1 } + # This is not a valid hashrocket if there's no previous tokens + return result if farrow_token.prev_token.nil? + if VALID_TOKEN_TYPES.include?(farrow_token.prev_token.type) + # Someone forgot the whitespace! e.g. ensure=> + result[:indent] = farrow_token.column + 1 + result[:name_token] = farrow_token.prev_token + return result + end + if farrow_token.prev_token.type == :WHITESPACE + # If the whitespace has no previous token (which shouldn't happen) or the thing before the whitespace is not a property name this it not a valid hashrocket + return result if farrow_token.prev_token.prev_token.nil? + return result unless VALID_TOKEN_TYPES.include?(farrow_token.prev_token.prev_token.type) + result[:name_token] = farrow_token.prev_token.prev_token + result[:indent] = farrow_token.prev_token.column + 1 # The indent is the whitespace column + 1 + end + result + end + end + end +end diff --git a/lib/puppet-languageserver/message_router.rb b/lib/puppet-languageserver/message_router.rb index d72e5dc2..b0574cbf 100644 --- a/lib/puppet-languageserver/message_router.rb +++ b/lib/puppet-languageserver/message_router.rb @@ -206,7 +206,31 @@ def receive_request(request) end when 'textDocument/onTypeFormatting' - request.reply_result(nil) + unless client.format_on_type + request.reply_result(nil) + return + end + file_uri = request.params['textDocument']['uri'] + line_num = request.params['position']['line'] + char_num = request.params['position']['character'] + content = documents.document(file_uri) + begin + case documents.document_type(file_uri) + when :manifest + request.reply_result(PuppetLanguageServer::Manifest::FormatOnTypeProvider.instance.format( + content, + line_num, + char_num, + request.params['ch'], + request.params['options'] + )) + else + raise "Unable to format on type on #{file_uri}" + end + rescue StandardError => e + PuppetLanguageServer.log_message(:error, "(textDocument/onTypeFormatting) #{e}") + request.reply_result(nil) + end when 'textDocument/signatureHelp' file_uri = request.params['textDocument']['uri'] diff --git a/lib/puppet-languageserver/providers.rb b/lib/puppet-languageserver/providers.rb index 874b08d9..01c38eb0 100644 --- a/lib/puppet-languageserver/providers.rb +++ b/lib/puppet-languageserver/providers.rb @@ -5,6 +5,7 @@ manifest/completion_provider manifest/definition_provider manifest/document_symbol_provider + manifest/format_on_type_provider manifest/signature_provider manifest/validation_provider manifest/hover_provider diff --git a/spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb b/spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb new file mode 100644 index 00000000..65320d2f --- /dev/null +++ b/spec/languageserver/unit/puppet-languageserver/manifest/format_on_type_provider_spec.rb @@ -0,0 +1,123 @@ +require 'spec_helper' + +describe 'PuppetLanguageServer::Manifest::FormatOnTypeProvider' do + let(:subject) { PuppetLanguageServer::Manifest::FormatOnTypeProvider.new } + + describe '::instance' do + it 'should exist' do + expect(PuppetLanguageServer::Manifest::FormatOnTypeProvider).to respond_to(:instance) + end + + it 'should return the same object' do + object1 = PuppetLanguageServer::Manifest::FormatOnTypeProvider.instance + object2 = PuppetLanguageServer::Manifest::FormatOnTypeProvider.instance + expect(object1).to eq(object2) + end + end + + describe '#format' do + let(:formatting_options) do + LSP::FormattingOptions.new.tap do |item| + item.tabSize = 2 + item.insertSpaces = true + end.to_h + end + + [' ', '=', ','].each do |trigger| + context "given a trigger character of '#{trigger}'" do + it 'should return an empty array' do + result = subject.format("{\n oneline =>\n}\n", 1, 1, trigger, formatting_options) + expect(result).to eq([]) + end + end + end + + context "given a trigger character of greater-than '>'" do + let(:trigger_character) { '>' } + let(:content) do <<-MANIFEST +user { + ensure=> 'something', + password => + name => { + 'abc' => '123', + 'def' => '789', + }, + name2 => 'correct', +} +MANIFEST + end + let(:valid_cursor) { { line: 2, char: 15 } } + let(:inside_cursor) { { line: 5, char: 15 } } + + it 'should return an empty array if the cursor is not on a hashrocket' do + result = subject.format(content, 1, 1, trigger_character, formatting_options) + expect(result).to eq([]) + end + + it 'should return an empty array if the formatting options uses tabs' do + result = subject.format(content, valid_cursor[:line], valid_cursor[:char], trigger_character, formatting_options.tap { |i| i['insertSpaces'] = false} ) + expect(result).to eq([]) + end + + it 'should return an empty array if the document is large' do + large_content = content + ' ' * 4096 + result = subject.format(large_content, valid_cursor[:line], valid_cursor[:char], trigger_character, formatting_options) + expect(result).to eq([]) + end + + # Valid hashrocket key tests + [ + { name: 'bare name', text: 'barename' }, + { name: 'single quoted string', text: '\'name\'' }, + { name: 'double quoted string', text: '"name"' }, + ].each do |testcase| + context "and given a manifest with #{testcase[:name]}" do + let(:content) { "{\n a =>\n ##TESTCASE## => 'value'\n}\n"} + + it 'should return an empty' do + result = subject.format(content.gsub('##TESTCASE##', testcase[:text]), 1, 6, trigger_character, formatting_options) + # The expected TextEdit should edit the `a =>` + expect(result.count).to eq(1) + expect(result[0].range.start.line).to eq(1) + expect(result[0].range.start.character).to eq(3) + expect(result[0].range.end.line).to eq(1) + expect(result[0].range.end.character).to eq(4) + end + end + end + + it 'should have valid text edits in the outer hash' do + result = subject.format(content, valid_cursor[:line], valid_cursor[:char], trigger_character, formatting_options) + + expect(result.count).to eq(3) + expect(result[0].to_h).to eq({"range"=>{"start"=>{"character"=>8, "line"=>1}, "end"=>{"character"=>8, "line"=>1}}, "newText"=>" "}) + expect(result[1].to_h).to eq({"range"=>{"start"=>{"character"=>10, "line"=>2}, "end"=>{"character"=>13, "line"=>2}}, "newText"=>" "}) + expect(result[2].to_h).to eq({"range"=>{"start"=>{"character"=>6, "line"=>3}, "end"=>{"character"=>7, "line"=>3}}, "newText"=>" "}) + end + + it 'should have valid text edits in the inner hash' do + result = subject.format(content, inside_cursor[:line], inside_cursor[:char], trigger_character, formatting_options) + + expect(result.count).to eq(1) + expect(result[0].to_h).to eq({"range"=>{"start"=>{"character"=>9, "line"=>5}, "end"=>{"character"=>13, "line"=>5}}, "newText"=>" "}) + end + + # Invalid scenarios + [ + { name: 'only one line', content: "{\n oneline =>\n}\n" }, + { name: 'there is nothing to indent', content: "{\n oneline =>\n nextline12 => 'value',\n}\n" }, + { name: 'no starting Left Brace', content: "\n oneline =>\n nextline12 => 'value',\n}\n" }, + { name: 'no ending Right Brace', content: "{\n oneline =>\n nextline12 => 'value',\n\n" }, + { name: 'hashrockets on the same line', content: "{\n oneline => , nextline12 => 'value',\n\n"}, + { name: 'invalid text before the hashrocket', content: "{\n String[] =>\n nextline => 'value',\n}\n" }, + ].each do |testcase| + context "and given a manifest with #{testcase[:name]}" do + it 'should return an empty' do + result = subject.format(testcase[:content], 1, 15, trigger_character, formatting_options) + expect(result).to eq([]) + end + end + end + end + end +end diff --git a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb index 090039b1..829c389f 100644 --- a/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb +++ b/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb @@ -859,6 +859,7 @@ context 'given a textDocument/onTypeFormatting request' do let(:request_rpc_method) { 'textDocument/onTypeFormatting' } let(:file_uri) { MANIFEST_FILENAME } + let(:file_content) { "{\n a =>\n name => 'value'\n}\n" } let(:line_num) { 1 } let(:char_num) { 6 } let(:trigger_char) { '>' } @@ -874,11 +875,77 @@ 'ch' => trigger_char, 'options' => formatting_options } } + let(:provider) { PuppetLanguageServer::Manifest::FormatOnTypeProvider.new } - it 'should not log an error message' do - expect(PuppetLanguageServer).to_not receive(:log_message).with(:error,"Unknown RPC method #{request_rpc_method}") + before(:each) do + subject.documents.clear + subject.documents.set_document(file_uri,file_content, 0) + end - subject.receive_request(request) + context 'with client.format_on_type set to false' do + before(:each) do + allow(subject.client).to receive(:format_on_type).and_return(false) + end + + it 'should reply with nil' do + expect(request).to receive(:reply_result).with(nil) + subject.receive_request(request) + end + end + + context 'with client.format_on_type set to true' do + before(:each) do + allow(subject.client).to receive(:format_on_type).and_return(true) + end + + context 'for a file the server does not understand' do + let(:file_uri) { UNKNOWN_FILENAME } + + it 'should log an error message' do + expect(PuppetLanguageServer).to receive(:log_message).with(:error,/Unable to format on type on/) + + subject.receive_request(request) + end + + it 'should reply with nil' do + expect(request).to receive(:reply_result).with(nil) + + subject.receive_request(request) + end + end + + context 'for a puppet manifest file' do + let(:file_uri) { MANIFEST_FILENAME } + + before(:each) do + allow(PuppetLanguageServer::Manifest::FormatOnTypeProvider).to receive(:instance).and_return(provider) + end + + it 'should call format method on the Format On Type provider' do + expect(provider).to receive(:format) + .with(file_content, line_num, char_num, trigger_char, formatting_options).and_return('something') + + result = subject.receive_request(request) + end + + context 'and an error occurs during formatting' do + before(:each) do + expect(provider).to receive(:format).and_raise('MockError') + end + + it 'should log an error message' do + expect(PuppetLanguageServer).to receive(:log_message).with(:error,/MockError/) + + subject.receive_request(request) + end + + it 'should reply with nil' do + expect(request).to receive(:reply_result).with(nil) + + subject.receive_request(request) + end + end + end end end