Custom Spec parser to better enforce format

Provides a pretty error message, pointing to the error location in
the YAML source code, to help the user fix the error.

src/cli.cr

@@ -51,9 +51,13 @@ begin
 
 rescue ex : OptionParser::InvalidOption
   Shards.logger.fatal ex.message
-  exit -1
+  exit 1
+
+rescue ex : Shards::ParseError
+  ex.to_s(STDERR)
+  exit 1
 
 rescue ex : Shards::Error
   Shards.logger.error ex.message
-  exit -1
+  exit 1
 end

src/errors.cr

@@ -21,4 +21,40 @@ module Shards
       super "Unsupported #{ LOCK_FILENAME }. It was likely generated from a newer version of Shards."
     end
   end
+
+  class ParseError < Error
+    getter :input
+    getter :filename
+    getter :line_number
+    getter :column_number
+
+    def initialize(message, @input, @filename, line_number, column_number)
+      @line_number = line_number.to_i
+      @column_number = column_number.to_i
+      super message
+    end
+
+    def to_s(io)
+      io.puts "in #{ filename }:#{ line_number }: #{ self.class.name }: #{ message }"
+      io.puts
+
+      lines = input.split('\n')
+      from = line_number - 2
+      from = 0 if from < 0
+
+      lines[from .. line_number].each_with_index do |line, i|
+        io.puts "  #{ i + 1 }. #{ line }"
+      end
+
+      arrow = String.build do |s|
+        s << "     "
+        (column_number - 1).times { s << ' ' }
+        s << '^'
+      end
+      io.puts arrow.colorize(:green).bold
+      io.puts
+
+      io.flush
+    end
+  end
 end

src/resolvers/resolver.cr

@@ -36,7 +36,7 @@ module Shards
     abstract def installed_commit_hash
 
     def run_script(name)
-      if installed? && (command = spec.script(name))
+      if installed? && (command = spec.scripts[name]?)
         Shards.logger.info "#{name.capitalize} #{command}"
         Script.run(install_path, command)
       end

src/spec.cr

@@ -1,4 +1,6 @@
+require "colorize"
 require "yaml"
+require "./config"
 require "./dependency"
 require "./errors"
 
@@ -21,71 +23,130 @@ module Shards
       end
     end
 
-    # :nodoc:
-    class Dependencies < Array(Dependency)
-      def self.new(pull : YAML::PullParser)
-        pull.read_mapping_start
-        dependencies = new
+    def self.from_file(path, validate = false)
+      path = File.join(path, SPEC_FILENAME) if File.directory?(path)
+      raise Error.new("Missing #{ File.basename(path) }") unless File.exists?(path)
+      from_yaml(File.read(path), path, validate)
+    end
 
-        while pull.kind != YAML::EventKind::MAPPING_END
-          dependencies << Dependency.new(pull)
+    def self.from_yaml(input, filename = SPEC_FILENAME, validate = false)
+      parser = YAML::PullParser.new(input)
+      parser.read_stream do
+        parser.read_document do
+          new(parser, validate)
         end
-
-        pull.read_next
-        dependencies
-      #rescue ex : YAML::ParseException
-      #  raise Exception.new("Invalid dependencies definition at #{ ex.line_number }:#{ ex.column_number }")
       end
+    rescue ex : YAML::ParseException
+      raise ParseError.new(ex.message, input, filename, ex.line_number, ex.column_number)
+    ensure
+      parser.close if parser
     end
 
-    def self.from_file(path)
-      path = File.join(path, SPEC_FILENAME) if File.directory?(path)
-      raise Error.new("Missing #{ File.basename(path) }") unless File.exists?(path)
-      from_yaml(File.read(path))
-    end
+    getter! :name
+    getter! :version
+    getter :description
+    getter :license
+
+    # :nodoc:
+    def initialize(pull : YAML::PullParser, validate = false)
+      read_mapping(pull) do
+        case key = pull.read_scalar
+        when "name"
+          @name = pull.read_scalar
+        when "version"
+          @version = pull.read_scalar
+        when "description"
+          @description = pull.read_scalar
+        when "license"
+          @license = pull.read_scalar
+        when "authors"
+          read_sequence(pull) do
+            authors << Author.new(pull.read_scalar)
+          end
+        when "dependencies"
+          read_mapping(pull) do
+            dependency = Dependency.new(pull.read_scalar)
+            read_mapping(pull) { dependency[pull.read_scalar] = pull.read_scalar }
+            dependencies << dependency
+          end
+        when "development_dependencies"
+          read_mapping(pull) do
+            dependency = Dependency.new(pull.read_scalar)
+            read_mapping(pull) { dependency[pull.read_scalar] = pull.read_scalar }
+            development_dependencies << dependency
+          end
+        when "scripts"
+          read_mapping(pull) do
+            scripts[pull.read_scalar] = pull.read_scalar
+          end
+        else
+          if validate
+            raise YAML::ParseException.new("unknown attribute: #{ key }", pull.line_number, pull.column_number)
+          else
+            pull.skip
+          end
+        end
+      end
 
-    yaml_mapping({
-      name: { type: String },
-      version: { type: String },
-      description: { type: String, nilable: true },
-      license: { type: String, nilable: true },
-      authors: { type: Array(Author), nilable: true },
-      scripts: { type: Hash(String, String), nilable: true },
-      dependencies: { type: Dependencies, nilable: true },
-      development_dependencies: { type: Dependencies, nilable: true },
-    })
+      {% for attr in %w(name version) %}
+        unless @{{ attr.id }}
+          raise YAML::ParseException.new(
+            "missing required attribute: {{ attr.id }}",
+            pull.line_number,
+            pull.column_number
+          )
+        end
+      {% end %}
+    end
 
-    getter! :version
+    def name=(@name : String)
+    end
 
-    def initialize(@name : String)
+    def version=(@version : String)
     end
 
     def authors
       @authors ||= [] of Author
     end
 
     def dependencies
-      @dependencies ||= Dependencies.new
+      @dependencies ||= [] of Dependency
     end
 
     def development_dependencies
-      @development_dependencies ||= Dependencies.new
+      @development_dependencies ||= [] of Dependency
     end
 
-    def script(name)
-      if scripts = self.scripts
-        scripts[name]?
-      end
+    def scripts
+      @scripts ||= {} of String => String
     end
 
     def license_url
-      if license = self.license
+      if license = @license
         if license =~ %r(https?://)
           license
         else
           "http://opensource.org/licenses/#{ license }"
         end
       end
     end
+
+    private def read_sequence(pull)
+      pull.read_sequence_start
+      until pull.kind == YAML::EventKind::SEQUENCE_END
+        yield
+      end
+      pull.read_next
+      nil
+    end
+
+    private def read_mapping(pull)
+      pull.read_mapping_start
+      until pull.kind == YAML::EventKind::MAPPING_END
+        yield
+      end
+      pull.read_next
+      nil
+    end
   end
 end

test/integration/install_test.cr

@@ -5,7 +5,6 @@ class InstallCommandTest < Minitest::Test
     metadata = {
       dependencies: { web: "*", orm: "*", },
       development_dependencies: { mock: "*" },
-      custom_dependencies: { release: "*" },
     }
 
     with_shard(metadata) do

test/integration/update_test.cr

@@ -5,7 +5,6 @@ class UpdateCommandTest < Minitest::Test
     metadata = {
       dependencies: { web: "*", orm: "*", },
       development_dependencies: { mock: "*" },
-      custom_dependencies: { release: "*" },
     }
 
     with_shard(metadata) do

test/spec_test.cr

@@ -1,5 +1,5 @@
 require "minitest/autorun"
-require "../src/spec"
+require "./test_helper"
 
 module Shards
   class SpecTest < Minitest::Test
@@ -99,15 +99,38 @@ module Shards
       assert_equal "master", spec.development_dependencies[1].refs
     end
 
-  #  def test_fails_to_parse_dependencies
-  #    str = <<-YAML
-  #name: amethyst
-  #version: 0.1.7
-  #dependencies:
-  #  github: spalger/crystal-mime
-  #  branch: master
-  #YAML
-  #    Spec.from_yaml(str)
-  #  end
+    def test_skips_unknown_attributes
+      spec = Spec.from_yaml("\nanme: test\ncustom:\n  test: more\nname: test\nversion: 1\n")
+      assert_equal "test", spec.name
+      assert_equal "1", spec.version
+
+      spec = Spec.from_yaml("\nanme:\nname: test\nversion: 1\n")
+      assert_equal "test", spec.name
+      assert_equal "1", spec.version
+    end
+
+    def test_raises_on_unknown_attributes_if_validating
+      ex = assert_raises(ParseError) { Spec.from_yaml("anme:", validate: true) }
+      assert_match "unknown attribute: anme", ex.message
+    end
+
+    def test_raises_when_required_attributes_are_missing
+      ex = assert_raises(ParseError) { Spec.from_yaml("license: MIT") }
+      assert_match "missing required attribute: name", ex.message
+
+      ex = assert_raises(ParseError) { Spec.from_yaml("name: test") }
+      assert_match "missing required attribute: version", ex.message
+    end
+
+    def test_fails_to_parse_dependencies
+      str = <<-YAML
+name: amethyst
+version: 0.1.7
+dependencies:
+  github: spalger/crystal-mime
+  branch: master
+YAML
+      ex = assert_raises(ParseError) { Spec.from_yaml(str) }
+    end
   end
 end