diff --git a/lib/yamllint/linter.rb b/lib/yamllint/linter.rb index 9823e4d..aef8316 100644 --- a/lib/yamllint/linter.rb +++ b/lib/yamllint/linter.rb @@ -96,7 +96,10 @@ def check_filename(filename) def check_data(yaml_data, errors_array) valid = check_not_empty?(yaml_data, errors_array) valid &&= check_syntax_valid?(yaml_data, errors_array) - valid &&= check_overlapping_keys?(yaml_data, errors_array) + valid &&= [ + check_overlapping_keys?(yaml_data, errors_array), + check_quoting_valid?(yaml_data, errors_array) + ].all? {|valid| valid} valid end @@ -124,30 +127,41 @@ def check_syntax_valid?(yaml_data, errors_array) end ### - # Detects duplicate keys in Psych parsed data + # Checks its violations or conventions in Psych parsed data recursivley # - class KeyOverlapDetector - attr_reader :overlapping_keys - - # Setup class variables + class RecursiveChecker + # Should have a readable attribute that has results of checking def initialize - @seen_keys = Set.new @key_components = [] - @last_key = [''] - @overlapping_keys = Set.new @complex_type = [] @array_positions = [] + @last_key = [''] end - # Get the data and send it off for duplicate key validation def parse(psych_parse_data) data_start = psych_parse_data.handler.root.children[0] parse_recurse(data_start) end private + + def check_on_value(node, key) + @current_node = node + YamlLint.logger.debug { "check_on_value: #{@current_node.value.inspect}, #{key.inspect}" } + + case @complex_type.last + when :hash + @key_components.push(key) + check! + @key_components.pop + when :array + @key_components.push(@array_positions.last) + check! + @array_positions[-1] += 1 + @key_components.pop + end + end - # Recusively check for duplicate keys def parse_recurse(psych_parse_data, is_sequence = false) is_key = false psych_parse_data.children.each do |n| @@ -155,7 +169,7 @@ def parse_recurse(psych_parse_data, is_sequence = false) when 'Psych::Nodes::Scalar' is_key = !is_key unless is_sequence @last_key.push(n.value) if is_key - add_value(n.value, @last_key.last) unless is_key + check_on_value(n, @last_key.last) unless is_key msg = "Scalar: #{n.value}, key: #{is_key}, last_key: #{@last_key}" YamlLint.logger.debug { msg } @last_key.pop if !is_key && !is_sequence @@ -184,7 +198,6 @@ def hash_start(key) complex_type_start(key) @complex_type.push(:hash) - check_for_overlap! end # Tear down a hash @@ -203,7 +216,6 @@ def array_start(key) @complex_type.push(:array) @array_positions.push(0) - check_for_overlap! end # Tear down the array @@ -215,25 +227,45 @@ def array_end(key) @array_positions.pop end - # Add a key / value pair - def add_value(value, key) - YamlLint.logger.debug { "add_value: #{value.inspect}, #{key.inspect}" } - + # Setup common hash and array elements + def complex_type_start(key) case @complex_type.last when :hash @key_components.push(key) - check_for_overlap! - @key_components.pop when :array @key_components.push(@array_positions.last) - check_for_overlap! @array_positions[-1] += 1 - @key_components.pop end end + end + + ### + # Detects duplicate keys in Psych parsed data + # + class KeyOverlapDetector < RecursiveChecker + attr_reader :overlapping_keys + + # Setup class variables + def initialize + super + @seen_keys = Set.new + @overlapping_keys = Set.new + end + + private + + def hash_start(key) + super(key) + check! + end + + def array_start(key) + super(key) + check! + end # Check for key overlap - def check_for_overlap! + def check! full_key = @key_components.dup YamlLint.logger.debug { "Checking #{full_key.join('.')} for overlap" } @@ -241,17 +273,6 @@ def check_for_overlap! YamlLint.logger.debug { "Overlapping key #{full_key.join('.')}" } @overlapping_keys << full_key end - - # Setup common hash and array elements - def complex_type_start(key) - case @complex_type.last - when :hash - @key_components.push(key) - when :array - @key_components.push(@array_positions.last) - @array_positions[-1] += 1 - end - end end # Check if there is overlapping key @@ -268,5 +289,87 @@ def check_overlapping_keys?(yaml_data, errors_array) overlap_detector.overlapping_keys.empty? end + + ### + # Check conventions for quoting of strings + # + class QuotingChecker < RecursiveChecker + attr_reader :single_quoted_strings, :double_quoted_strings + + # Setup class variables + def initialize + super + @single_quoted_strings = [] + @double_quoted_strings = [] + end + + private + + def check! + node = @current_node + return unless node.quoted + YamlLint.logger.debug { "Checking #{quoted_value(node)} for quoting conventions" } + + full_key = @key_components.dup + + case node.style + when Psych::Nodes::Scalar::SINGLE_QUOTED + if include_esacpe_characters?(node) + YamlLint.logger.debug { "Escape characters in single quoted string #{quoted_value(node)}" } + @single_quoted_strings << ["#{full_key.join('.')}", quoted_value(node)] + end + when Psych::Nodes::Scalar::DOUBLE_QUOTED + unless include_esacpe_characters?(node) || include_single_quote?(node) + YamlLint.logger.debug { "Double quoted string without escape characters or single quotes(') #{quoted_value(node)}" } + @double_quoted_strings << ["#{full_key.join('.')}", quoted_value(node)] + end + end + end + + def quoted_value(node) + case node.style + when Psych::Nodes::Scalar::SINGLE_QUOTED + "\'#{node.value}\'" + when Psych::Nodes::Scalar::DOUBLE_QUOTED + "\"#{node.value}\"" + end + end + + def unescaped_string(node) + case node.style + when Psych::Nodes::Scalar::SINGLE_QUOTED + node.value.scrub + when Psych::Nodes::Scalar::DOUBLE_QUOTED + node.value.inspect[1...-1].gsub('\\\\', '\\').scrub + end + end + + def include_esacpe_characters?(node) + unescaped_string(node) =~ /\\[abefnrtv]{0,1}|\\[0-7]{1,3}|\\x[0-9a-fA-F]{1,2}|\\u[0-9a-fA-F]{1,6}/ + end + + def include_single_quote?(node) + unescaped_string(node).include?('\'') + end + end + + def check_quoting_valid?(yaml_data, errors_array) + quoting_checker = QuotingChecker.new + data = Psych.parser.parse(yaml_data) + + quoting_checker.parse(data) + + quoting_checker.single_quoted_strings.each do |key, string| + err_meg = "Use double quoted strings if you need to parse escape characters: #{key}: #{string}" + errors_array << err_meg + end + + quoting_checker.double_quoted_strings.each do |key, string| + err_meg = "Prefer single-quoted strings when you don't need to parse escape characters: #{key}: #{string}" + errors_array << err_meg + end + + quoting_checker.single_quoted_strings.empty? && quoting_checker.double_quoted_strings.empty? + end end end diff --git a/spec/cli_spec.rb b/spec/cli_spec.rb index 223b3e3..03f5758 100644 --- a/spec/cli_spec.rb +++ b/spec/cli_spec.rb @@ -34,14 +34,14 @@ it '-D should print debug log and exit successfully with good YAML' do yamllint spec_data('valid.yaml -D') expect(last_command_started).to be_successfully_executed - expect(last_command_started).to have_output(/DEBUG -- : add_value: "bar"/) + expect(last_command_started).to have_output(/DEBUG -- : check_on_value: "bar"/) expect(last_command_started).to have_output(/DEBUG -- : Checking my_array/) end it '--debug should print debug log and exit successfully with good YAML' do yamllint spec_data('valid.yaml --debug') expect(last_command_started).to be_successfully_executed - expect(last_command_started).to have_output(/DEBUG -- : add_value: "bar"/) + expect(last_command_started).to have_output(/DEBUG -- : check_on_value: "bar"/) expect(last_command_started).to have_output(/DEBUG -- : Checking my_array/) end diff --git a/spec/data/double_quote_in_single_quotes.yaml b/spec/data/double_quote_in_single_quotes.yaml new file mode 100644 index 0000000..07ec950 --- /dev/null +++ b/spec/data/double_quote_in_single_quotes.yaml @@ -0,0 +1,2 @@ +--- +foo: '"' diff --git a/spec/data/double_quoted_no_escape_character.yaml b/spec/data/double_quoted_no_escape_character.yaml new file mode 100644 index 0000000..c4dd98e --- /dev/null +++ b/spec/data/double_quoted_no_escape_character.yaml @@ -0,0 +1,2 @@ +--- +foo: "bar" diff --git a/spec/data/invalid_with_double_quote.yaml b/spec/data/invalid_with_double_quote.yaml new file mode 100644 index 0000000..89ac03e --- /dev/null +++ b/spec/data/invalid_with_double_quote.yaml @@ -0,0 +1,2 @@ +--- +foo: """ diff --git a/spec/data/invalid_with_single_quote.yaml b/spec/data/invalid_with_single_quote.yaml new file mode 100644 index 0000000..5676b31 --- /dev/null +++ b/spec/data/invalid_with_single_quote.yaml @@ -0,0 +1,2 @@ +--- +foo: ''' diff --git a/spec/data/single_quote_in_double_quotes.yaml b/spec/data/single_quote_in_double_quotes.yaml new file mode 100644 index 0000000..64a4c86 --- /dev/null +++ b/spec/data/single_quote_in_double_quotes.yaml @@ -0,0 +1,2 @@ +--- +foo: "'" diff --git a/spec/data/single_quoted_escape_character.yaml b/spec/data/single_quoted_escape_character.yaml new file mode 100644 index 0000000..26622b3 --- /dev/null +++ b/spec/data/single_quoted_escape_character.yaml @@ -0,0 +1,2 @@ +--- +foo: 'bar\n' diff --git a/spec/data/valid_complex.yaml b/spec/data/valid_complex.yaml index a0a2b21..c1758e7 100644 --- a/spec/data/valid_complex.yaml +++ b/spec/data/valid_complex.yaml @@ -10,7 +10,7 @@ YAML Resources: YAML 1.0 (1st Edition): http://yaml.org/spec/1.0/ YAML Issues Page: https://github.com/yaml/yaml/issues YAML Mailing List: yaml-core@lists.sourceforge.net - YAML IRC Channel: "#yaml on irc.freenode.net" + YAML IRC Channel: '#yaml on irc.freenode.net' YAML Cookbook (Ruby): http://yaml4r.sourceforge.net/cookbook/ (local) YAML Reference Parser: http://yaml.org/ypaste/ diff --git a/spec/linter_spec.rb b/spec/linter_spec.rb index 3442b59..de5a6a8 100644 --- a/spec/linter_spec.rb +++ b/spec/linter_spec.rb @@ -70,4 +70,28 @@ it 'should be unhapy with a YAML file full of spaces' do expect(linter.check(spec_data('spaces.yaml'))).to be(false) end + + it 'should be unhappy with a YAML that has an escape charater in single quoted string' do + expect(linter.check(spec_data('single_quoted_escape_character.yaml'))).to be(false) + end + + it 'should be unhappy with a YAML that has a double quoted string without escape charaters' do + expect(linter.check(spec_data('double_quoted_no_escape_character.yaml'))).to be(false) + end + + it 'should be happy with a YAML that has a double quote in single quoted string' do + expect(linter.check(spec_data('double_quote_in_single_quotes.yaml'))).to be(true) + end + + it 'should be happy with a YAML that has a single quote in double quoted string' do + expect(linter.check(spec_data('single_quote_in_double_quotes.yaml'))).to be(true) + end + + it 'should be unhappy with a YAML that has a single quote in single quoted string' do + expect(linter.check(spec_data('invalid_with_single_quote.yaml'))).to be(false) + end + + it 'should be unhappy with a YAML that has a double quote in double quoted string' do + expect(linter.check(spec_data('invalid_with_double_quote.yaml'))).to be(false) + end end