YAML parser for Dependency and Target (crystal-lang#306)

* Refactor Dependency and Target with proper YAML parser
* Refactor git and path properties on Dependency
* Remove specialized git resolvers (GitHub, GitLab, BitBucket) in favour of handling generic git urls

spec/unit/dependency_spec.cr

@@ -4,27 +4,31 @@ module Shards
   describe Dependency do
     it "version" do
       dependency = Dependency.new("app")
-      dependency.version.should eq("*")
-
-      dependency = Dependency.new("app", {version: "*"})
-      dependency.version.should eq("*")
+      dependency.version?.should be_nil
 
-      dependency = Dependency.new("app", {version: "1.0.0"})
-      dependency.version.should eq("1.0.0")
+      dependency = Dependency.new("app")
+      dependency.version = "<= 1.0.0"
+      dependency.version?.should eq("<= 1.0.0")
+      dependency.version.should eq("<= 1.0.0")
 
-      dependency = Dependency.new("app", {version: "<= 2.0.0"})
+      dependency = Dependency.new("app")
+      dependency.version = "<= 2.0.0"
+      dependency.version?.should eq("<= 2.0.0")
       dependency.version.should eq("<= 2.0.0")
     end
 
     it "version with tags" do
-      dependency = Dependency.new("app", {tag: "fix/something"})
+      dependency = Dependency.new("app")
+      dependency.tag = "fix/something"
       dependency.version.should eq("*")
 
-      dependency = Dependency.new("app", {tag: "1.2.3"})
+      dependency = Dependency.new("app")
+      dependency.tag = "1.2.3"
       dependency.version.should eq("*")
 
       # version tag is considered a version:
-      dependency = Dependency.new("app", {tag: "v1.2.3-pre1"})
+      dependency = Dependency.new("app")
+      dependency.tag = "v1.2.3-pre1"
       dependency.version.should eq("1.2.3-pre1")
     end
   end

spec/unit/git_resolver_spec.cr

@@ -1,8 +1,7 @@
 require "./spec_helper"
 
-private def resolver(name, config = {} of String => String)
-  config = config.merge({"git" => git_url(name)})
-  dependency = Shards::Dependency.from_name_config(name, config)
+private def resolver(name)
+  dependency = Shards::Dependency.new(name, git: git_url(name))
   Shards::GitResolver.new(dependency)
 end
 
@@ -45,15 +44,15 @@ module Shards
     end
 
     it "origin changed" do
-      dependency = Dependency.new("library", {"git" => git_url("library")})
+      dependency = Dependency.new("library", git: git_url("library"))
       library = GitResolver.new(dependency)
       library.install("0.1.2")
 
       # Change the origin in the cache repo to https://github.com/foo/bar
       Dir.cd(library.local_path) do
         run "git remote set-url origin https://github.com/foo/bar"
       end
-
+      #
       # All of these alternatives should not trigger origin as changed
       same_origins = [
         "https://github.com/foo/bar",
@@ -68,7 +67,7 @@ module Shards
       ]
 
       same_origins.each do |origin|
-        dependency["git"] = origin
+        dependency.git = origin
         library.origin_changed?.should be_false
       end
 
@@ -84,7 +83,7 @@ module Shards
       ]
 
       changed_origins.each do |origin|
-        dependency["git"] = origin
+        dependency.git = origin
         library.origin_changed?.should be_true
       end
     end

spec/unit/lock_spec.cr

@@ -18,11 +18,13 @@ module Shards
       shards.size.should eq(2)
 
       shards[0].name.should eq("repo")
-      shards[0]["github"].should eq("user/repo")
+      shards[0].path.should be_nil
+      shards[0].git.should eq("https://github.com/user/repo.git")
       shards[0].version.should eq("1.2.3")
 
       shards[1].name.should eq("example")
-      shards[1]["git"].should eq("https://example.com/example-crystal.git")
+      shards[1].path.should be_nil
+      shards[1].git.should eq("https://example.com/example-crystal.git")
       shards[1].refs.should eq("0d246ee6c52d4e758651b8669a303f04be9a2a96")
     end
 

spec/unit/path_resolver_spec.cr

@@ -1,8 +1,7 @@
 require "./spec_helper"
 
-private def resolver(name, config = {} of String => String)
-  config["path"] = git_path(name)
-  dependency = Shards::Dependency.from_name_config(name, config)
+private def resolver(name)
+  dependency = Shards::Dependency.new(name, path: git_path(name))
   Shards::PathResolver.new(dependency)
 end
 

spec/unit/resolver_spec.cr

@@ -3,10 +3,7 @@ require "./spec_helper"
 module Shards
   describe Resolver do
     it "find resolver with unordered dependency keys" do
-      dependency = Dependency.new("test", {
-        "branch" => "master",
-        "git"    => "file:///tmp/test",
-      })
+      dependency = Dependency.new("test", git: "file:///tmp/test")
       Shards.find_resolver(dependency).class.should eq(GitResolver)
     end
   end

spec/unit/spec_helper.cr

@@ -10,12 +10,6 @@ require "../support/factories"
 
 module Shards
   set_warning_log_level
-
-  class Dependency
-    def self.from_name_config(name, config) : self
-      Dependency.new(name, config)
-    end
-  end
 end
 
 Spec.before_each do

spec/unit/spec_spec.cr

@@ -64,21 +64,62 @@ module Shards
       spec.dependencies.size.should eq(3)
 
       spec.dependencies[0].name.should eq("repo")
-      spec.dependencies[0]["github"].should eq("user/repo")
+      spec.dependencies[0].path.should be_nil
+      spec.dependencies[0].git.should eq("https://github.com/user/repo.git")
       spec.dependencies[0].version.should eq("1.2.3")
       spec.dependencies[0].refs.should be_nil
 
       spec.dependencies[1].name.should eq("example")
-      spec.dependencies[1]["git"].should eq("https://example.com/example-crystal.git")
+      spec.dependencies[1].path.should be_nil
+      spec.dependencies[1].git.should eq("https://example.com/example-crystal.git")
       spec.dependencies[1].version.should eq("*")
       spec.dependencies[1].refs.should eq("master")
 
       spec.dependencies[2].name.should eq("local")
-      spec.dependencies[2]["path"].should eq("/var/clones/local")
+      spec.dependencies[2].path.should eq("/var/clones/local")
+      spec.dependencies[2].git.should be_nil
       spec.dependencies[2].version.should eq("*")
       spec.dependencies[2].refs.should eq("unreleased")
     end
 
+    it "fails dependency with duplicate resolver" do
+      expect_raises Shards::ParseError, %(Duplicate resolver mapping for dependency "foo" at line 6, column 5) do
+        Spec.from_yaml <<-YAML
+          name: orm
+          version: 1.0.0
+          dependencies:
+            foo:
+              github: user/repo
+              gitlab: user/repo
+          YAML
+      end
+    end
+
+    it "fails dependency with missing resolver" do
+      expect_raises Shards::ParseError, %(Missing resolver for dependency "foo" at line 4, column 3) do
+        Spec.from_yaml <<-YAML
+          name: orm
+          version: 1.0.0
+          dependencies:
+            foo:
+              branch: master
+          YAML
+      end
+    end
+
+    it "accepts dependency with extra attributes" do
+      spec = Spec.from_yaml <<-YAML
+        name: orm
+        version: 1.0.0
+        dependencies:
+          foo:
+            github: user/repo
+            extra: foobar
+        YAML
+      dep = Dependency.new("foo", git: "https://github.com/user/repo.git")
+      spec.dependencies[0].should eq dep
+    end
+
     it "parse development dependencies" do
       spec = Spec.from_yaml <<-YAML
       name: orm
@@ -95,11 +136,13 @@ module Shards
       spec.development_dependencies.size.should eq(2)
 
       spec.development_dependencies[0].name.should eq("minitest")
-      spec.development_dependencies[0]["github"].should eq("ysbaddaden/minitest.cr")
+      spec.development_dependencies[0].path.should be_nil
+      spec.development_dependencies[0].git.should eq("https://github.com/ysbaddaden/minitest.cr.git")
       spec.development_dependencies[0].version.should eq("0.1.4")
 
       spec.development_dependencies[1].name.should eq("webmock")
-      spec.development_dependencies[1]["git"].should eq("https://github.com/manastech/webcmok-crystal.git")
+      spec.development_dependencies[1].path.should be_nil
+      spec.development_dependencies[1].git.should eq("https://github.com/manastech/webcmok-crystal.git")
       spec.development_dependencies[1].refs.should eq("master")
     end
 
@@ -123,6 +166,18 @@ module Shards
       spec.targets[1].main.should eq("src/command/cli.cr")
     end
 
+    it "fails target missing main" do
+      expect_raises Shards::ParseError, %(Missing property "main" for target "foo" at line 4, column 3) do
+        Spec.from_yaml <<-YAML
+          name: orm
+          version: 1.0.0
+          targets:
+            foo:
+              foo: bar
+          YAML
+      end
+    end
+
     it "parse executables" do
       spec = Spec.from_yaml <<-YAML
       name: test

src/commands/check.cr

@@ -42,7 +42,7 @@ module Shards
           return false
         end
 
-        if version = lock["version"]?
+        if version = lock.version { nil }
           if Versions.resolve([version], dependency.version).empty?
             Log.debug { "#{dependency.name}: lock conflict" }
             return false

src/commands/install.cr

@@ -33,7 +33,7 @@ module Shards
       private def validate(packages)
         packages.each do |package|
           if lock = locks.find { |d| d.name == package.name }
-            if commit = lock["commit"]?
+            if commit = lock.commit
               validate_locked_commit(package, commit)
             elsif version = lock.version?
               validate_locked_version(package, version)
@@ -86,7 +86,7 @@ module Shards
 
       private def outdated_lockfile?(packages)
         a = packages.map { |x| {x.name, x.version, x.commit} }
-        b = locks.map { |x| {x.name, x["version"]?, x["commit"]?} }
+        b = locks.map { |x| {x.name, x.version?, x.commit} }
         a != b
       end
     end

src/dependency.cr

@@ -1,26 +1,71 @@
 require "./ext/yaml"
 
 module Shards
-  class Dependency < Hash(String, String)
+  class Dependency
     property name : String
+    setter version : String?
+    property path : String?
+    property git : String?
+    property tag : String?
+    property branch : String?
+    property commit : String?
 
     def self.new(pull : YAML::PullParser) : self
-      Dependency.new(pull.read_scalar).tap do |dependency|
-        pull.each_in_mapping do
-          dependency[pull.read_scalar] = pull.read_scalar
+      start_pos = pull.location
+      dependency = Dependency.new(pull.read_scalar)
+
+      pull.each_in_mapping do
+        mapping_start = pull.location
+        case key = pull.read_scalar
+        when "version"
+          dependency.version = pull.read_scalar
+        when "tag"
+          dependency.tag = pull.read_scalar
+        when "branch"
+          dependency.branch = pull.read_scalar
+        when "commit"
+          dependency.commit = pull.read_scalar
+        when "path"
+          if dependency.path || dependency.git
+            raise YAML::ParseException.new("Duplicate resolver mapping for dependency #{dependency.name.inspect}", *mapping_start)
+          end
+
+          dependency.path = pull.read_scalar
+        when "git", "github", "gitlab", "bitbucket"
+          if dependency.path || dependency.git
+            raise YAML::ParseException.new("Duplicate resolver mapping for dependency #{dependency.name.inspect}", *mapping_start)
+          end
+
+          dependency.git = GitResolver.expand_resolver_url(pull.read_scalar, key)
+        else
+          # ignore unknown dependency mapping for future extensions
         end
       end
+
+      unless dependency.git || dependency.path
+        raise YAML::ParseException.new("Missing resolver for dependency #{dependency.name.inspect}", *start_pos)
+      end
+
+      dependency
     end
 
-    protected def initialize(@name)
-      super()
+    def self.new(name, *, git)
+      new(name).tap do |dependency|
+        dependency.git = git
+      end
+    end
+
+    def self.new(name, *, path)
+      new(name).tap do |dependency|
+        dependency.path = path
+      end
     end
 
-    protected def initialize(@name, config)
-      super()
-      config.each { |k, v| self[k.to_s] = v.to_s }
+    def initialize(@name)
     end
 
+    def_equals_and_hash @name, @version, @git, @path, @tag, @branch, @commit
+
     def version
       version { "*" }
     end
@@ -29,36 +74,32 @@ module Shards
       version { nil }
     end
 
-    def prerelease?
-      Versions.prerelease? version
-    end
-
-    private def version
-      if version = self["version"]?
+    def version
+      if version = @version
         version
-      elsif self["tag"]? =~ VERSION_TAG
+      elsif tag =~ VERSION_TAG
         $1
       else
         yield
       end
     end
 
-    def refs
-      self["branch"]? || self["tag"]? || self["commit"]?
+    def prerelease?
+      Versions.prerelease? version
     end
 
-    def path
-      self["path"]?
+    def refs
+      branch || tag || commit
     end
 
     def to_human_requirement
-      if version = version?
+      if version?
         version
-      elsif branch = self["branch"]?
+      elsif branch
         "branch #{branch}"
-      elsif tag = self["tag"]?
+      elsif tag
         "tag #{tag}"
-      elsif commit = self["commit"]?
+      elsif commit
         "commit #{commit}"
       else
         "*"