From 9de0065991dcb6f0ff81b3f369ca8dcdba659026 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Mon, 25 Nov 2019 13:58:40 +0100
Subject: [PATCH] fixup

---
 src/commands/build.cr      |  4 +--
 src/commands/check.cr      |  2 +-
 src/dependency.cr          | 34 ++++++++++---------
 src/lock.cr                |  2 +-
 src/resolvers/resolver.cr  |  2 +-
 src/target.cr              | 22 ++++---------
 test/dependency_test.cr    | 18 ++++------
 test/git_resolver_test.cr  |  7 ++--
 test/lock_test.cr          |  4 +--
 test/path_resolver_test.cr |  7 ++--
 test/resolver_test.cr      |  8 +++--
 test/spec_test.cr          | 67 ++++++++++++++++++++++++++++++++++----
 12 files changed, 110 insertions(+), 67 deletions(-)

diff --git a/src/commands/build.cr b/src/commands/build.cr
index 2fed759c..dd9d4598 100644
--- a/src/commands/build.cr
+++ b/src/commands/build.cr
@@ -27,8 +27,8 @@ module Shards
 
         args = [
           "build",
-          "-o", target.output_path,
-          target.source_path,
+          "-o", File.join(Shards.bin_path, target.name),
+          target.main,
         ]
         options.each { |option| args << option }
         Shards.logger.debug { "crystal #{args.join(' ')}" }
diff --git a/src/commands/check.cr b/src/commands/check.cr
index e5be22e6..dd340df0 100644
--- a/src/commands/check.cr
+++ b/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?
             Shards.logger.debug { "#{dependency.name}: lock conflict" }
             return false
diff --git a/src/dependency.cr b/src/dependency.cr
index e52a92b3..0e62ca54 100644
--- a/src/dependency.cr
+++ b/src/dependency.cr
@@ -4,18 +4,18 @@ module Shards
   class Dependency
     property name : String
     setter version : String?
-    property! resolver : String?
+    property! resolver_name : String?
     property! url : String?
     property tag : String?
     property branch : String?
     property commit : String?
 
-    getter unmapped = Hash(String, YAML::Any).new
-
     def self.new(pull : YAML::PullParser) : self
+      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
@@ -28,41 +28,43 @@ module Shards
         when "commit"
           dependency.commit = pull.read_scalar
         when "path", "git", "github", "gitlab", "bitbucket"
-          if (resolver = dependency.resolver?) && resolver != key
-            dependency.unmapped[resolver] = YAML::Any.new(dependency.url)
+          if dependency.resolver_name?
+            raise YAML::ParseException.new("Duplicate resolver mapping for dependency #{dependency.name.inspect}", *mapping_start)
           end
-          dependency.resolver = key
+          dependency.resolver_name = key
           dependency.url = pull.read_scalar
         else
-          dependency.unmapped[key] = YAML::Any.new(pull.read_scalar)
+          # ignore unknown dependency mapping for future extensions
         end
       end
 
       unless dependency.url?
-        raise "Invalid dependency, missing resolver"
+        raise YAML::ParseException.new("Missing resolver for dependency #{dependency.name.inspect}", *start_pos)
       end
 
       dependency
     end
 
-    def initialize(
-      @name : String,
-      @resolver : String? = nil, @url : String? = nil,
-      @version : String? = nil,
-      @branch : String? = nil, @tag : String? = nil, @commit : String? = nil,
-      @unmapped : Hash(String, YAML::Any) = Hash(String, YAML::Any).new
-    )
+    def initialize(@name)
     end
 
+    def_equals_and_hash @name, @version, @resolver_name, @url, @tag, @branch, @commit
+
     def version
-      version? || "*"
+      version { "*" }
     end
 
     def version?
+      version { nil }
+    end
+
+    def version
       if version = @version
         version
       elsif tag =~ VERSION_TAG
         $1
+      else
+        yield
       end
     end
 
diff --git a/src/lock.cr b/src/lock.cr
index 1dc8efc2..4ad6dbf1 100644
--- a/src/lock.cr
+++ b/src/lock.cr
@@ -56,7 +56,7 @@ module Shards
         dependency = package.resolver.dependency
 
         io << "  " << package.name << ":\n"
-        io << "    " << dependency.resolver << ": " << dependency.url << '\n'
+        io << "    " << dependency.resolver_name << ": " << dependency.url << '\n'
 
         if package.commit
           io << "    commit: " << package.commit << '\n'
diff --git a/src/resolvers/resolver.cr b/src/resolvers/resolver.cr
index 09f939b8..394a8839 100644
--- a/src/resolvers/resolver.cr
+++ b/src/resolvers/resolver.cr
@@ -66,7 +66,7 @@ module Shards
 
   def self.find_resolver(dependency)
     @@resolvers[dependency.name] ||= begin
-      klass = @@resolver_classes[dependency.resolver]?
+      klass = @@resolver_classes[dependency.resolver_name]?
       raise Error.new("Failed can't resolve dependency #{dependency.name} (unsupported resolver)") unless klass
       klass.new(dependency)
     end
diff --git a/src/target.cr b/src/target.cr
index 38075a98..19810d7c 100644
--- a/src/target.cr
+++ b/src/target.cr
@@ -1,40 +1,30 @@
 module Shards
   class Target
     property name : String
-    property main : String? = nil
-
-    getter attributes : Hash(String, YAML::Any)
+    property main : String
 
     def self.new(pull : YAML::PullParser) : self
+      start_pos = pull.location
       name = pull.read_scalar
       main = nil
-      extra_attributes = {} of String => YAML::Any
 
       pull.each_in_mapping do
         case key = pull.read_scalar
         when "main"
           main = pull.read_scalar
         else
-          extra_attributes[key] = YAML::Any.new(pull.read_scalar)
+          # ignore unknown dependency mapping for future extensions
         end
       end
 
       unless main
-        raise "Missing attribute 'main' for target #{name.inspect}"
+        raise YAML::ParseException.new(%(Missing property "main" for target #{name.inspect}), *start_pos)
       end
 
-      Target.new(name, main, extra_attributes)
-    end
-
-    def initialize(@name : String, @main : String, @attributes = {} of String => YAML::Any)
-    end
-
-    def source_path : String
-      main
+      Target.new(name, main)
     end
 
-    def output_path : String
-      File.join(Shards.bin_path, name)
+    def initialize(@name, @main)
     end
   end
 end
diff --git a/test/dependency_test.cr b/test/dependency_test.cr
index e83da011..b3d49437 100644
--- a/test/dependency_test.cr
+++ b/test/dependency_test.cr
@@ -3,27 +3,21 @@ require "./test_helper"
 class Shards::DependencyTest < Minitest::Test
   def test_version
     dependency = Dependency.new("app")
+    assert_nil dependency.version?
     assert_equal "*", dependency.version
 
-    dependency = Dependency.new("app", version: "*")
-    assert_equal "*", dependency.version
-
-    dependency = Dependency.new("app", version: "1.0.0")
-    assert_equal "1.0.0", dependency.version
-
-    dependency = Dependency.new("app", version: "<= 2.0.0")
+    dependency.version = "<= 2.0.0"
+    assert_equal "<= 2.0.0", dependency.version?
     assert_equal "<= 2.0.0", dependency.version
   end
 
   def test_version_with_tags
-    dependency = Dependency.new("app", tag: "fix/something")
-    assert_equal "*", dependency.version
+    dependency = Dependency.new("app")
 
-    dependency = Dependency.new("app", tag: "1.2.3")
+    dependency.tag = "fix/something"
     assert_equal "*", dependency.version
 
-    # version tag is considered a version:
-    dependency = Dependency.new("app", tag: "v1.2.3-pre1")
+    dependency.tag = "v1.2.3-pre1"
     assert_equal "1.2.3-pre1", dependency.version
   end
 end
diff --git a/test/git_resolver_test.cr b/test/git_resolver_test.cr
index 3e1c2813..5d030f14 100644
--- a/test/git_resolver_test.cr
+++ b/test/git_resolver_test.cr
@@ -44,9 +44,10 @@ module Shards
       skip "TODO: install tag (whatever the version)"
     end
 
-    private def resolver(name, **config)
-      config = config.merge(resolver: "git", url: git_url(name))
-      dependency = Dependency.new(name, **config)
+    private def resolver(name)
+      dependency = Dependency.new(name)
+      dependency.resolver_name = "git"
+      dependency.url = git_url(name)
       GitResolver.new(dependency)
     end
   end
diff --git a/test/lock_test.cr b/test/lock_test.cr
index 89a317bc..eb6c35d7 100644
--- a/test/lock_test.cr
+++ b/test/lock_test.cr
@@ -18,12 +18,12 @@ module Shards
       assert_equal 2, shards.size
 
       assert_equal "repo", shards[0].name
-      assert_equal "github", shards[0].resolver
+      assert_equal "github", shards[0].resolver_name
       assert_equal "user/repo", shards[0].url
       assert_equal "1.2.3", shards[0].version
 
       assert_equal "example", shards[1].name
-      assert_equal "git", shards[1].resolver
+      assert_equal "git", shards[1].resolver_name
       assert_equal "https://example.com/example-crystal.git", shards[1].url
       assert_equal "0d246ee6c52d4e758651b8669a303f04be9a2a96", shards[1].refs
     end
diff --git a/test/path_resolver_test.cr b/test/path_resolver_test.cr
index b1cafbe9..bbc03f52 100644
--- a/test/path_resolver_test.cr
+++ b/test/path_resolver_test.cr
@@ -74,9 +74,10 @@ module Shards
       end
     end
 
-    private def resolver(name, **config)
-      config = config.merge(resolver: "path", url: git_path(name))
-      dependency = Dependency.new(name, **config)
+    private def resolver(name)
+      dependency = Dependency.new(name)
+      dependency.resolver_name = "path"
+      dependency.url = git_path(name)
       PathResolver.new(dependency)
     end
   end
diff --git a/test/resolver_test.cr b/test/resolver_test.cr
index 44f2f31d..b7101a15 100644
--- a/test/resolver_test.cr
+++ b/test/resolver_test.cr
@@ -3,9 +3,11 @@ require "./test_helper"
 module Shards
   class ResolverTest < Minitest::Test
     def test_find_resolver_with_unordered_dependency_keys
-      dependency = Dependency.new("test", branch: "master",
-        resolver: "git", url: "file:///tmp/test",
-      )
+      dependency = Dependency.new("test")
+      dependency.branch = "master"
+      dependency.resolver_name = "git"
+      dependency.url = "file:///tmp/test"
+
       assert_equal GitResolver, Shards.find_resolver(dependency).class
     end
   end
diff --git a/test/spec_test.cr b/test/spec_test.cr
index 023fb80e..ceae0003 100644
--- a/test/spec_test.cr
+++ b/test/spec_test.cr
@@ -65,24 +65,66 @@ module Shards
       assert_equal 3, spec.dependencies.size
 
       assert_equal "repo", spec.dependencies[0].name
-      assert_equal "github", spec.dependencies[0].resolver
+      assert_equal "github", spec.dependencies[0].resolver_name
       assert_equal "user/repo", spec.dependencies[0].url
       assert_equal "1.2.3", spec.dependencies[0].version
       assert_nil spec.dependencies[0].refs
 
       assert_equal "example", spec.dependencies[1].name
-      assert_equal "git", spec.dependencies[1].resolver
+      assert_equal "git", spec.dependencies[1].resolver_name
       assert_equal "https://example.com/example-crystal.git", spec.dependencies[1].url
       assert_equal "*", spec.dependencies[1].version
       assert_equal "master", spec.dependencies[1].refs
 
       assert_equal "local", spec.dependencies[2].name
-      assert_equal "path", spec.dependencies[2].resolver
+      assert_equal "path", spec.dependencies[2].resolver_name
       assert_equal "/var/clones/local", spec.dependencies[2].url
       assert_equal "*", spec.dependencies[2].version
       assert_equal "unreleased", spec.dependencies[2].refs
     end
 
+    def test_dependency_with_duplicate_resolver
+      exc = assert_raises Shards::ParseError do
+        Spec.from_yaml <<-YAML
+          name: orm
+          version: 1.0.0
+          dependencies:
+            foo:
+              github: user/repo
+              gitlab: user/repo
+          YAML
+      end
+      assert_equal %(Duplicate resolver mapping for dependency "foo" at line 6, column 5), exc.message
+    end
+
+    def test_dependency_missing_resolver
+      exc = assert_raises Shards::ParseError do
+        Spec.from_yaml <<-YAML
+          name: orm
+          version: 1.0.0
+          dependencies:
+            foo:
+              branch: master
+          YAML
+      end
+      assert_equal %(Missing resolver for dependency "foo" at line 4, column 3), exc.message
+    end
+
+    def test_dependency_with_extra_attributes
+      spec = Spec.from_yaml <<-YAML
+        name: orm
+        version: 1.0.0
+        dependencies:
+          foo:
+            github: user/repo
+            extra: foobar
+        YAML
+      dep = Dependency.new("foo")
+      dep.resolver_name = "github"
+      dep.url = "user/repo"
+      assert_equal dep, spec.dependencies[0]
+    end
+
     def test_parse_development_dependencies
       spec = Spec.from_yaml <<-YAML
       name: orm
@@ -99,12 +141,12 @@ module Shards
       assert_equal 2, spec.development_dependencies.size
 
       assert_equal "minitest", spec.development_dependencies[0].name
-      assert_equal "github", spec.development_dependencies[0].resolver
+      assert_equal "github", spec.development_dependencies[0].resolver_name
       assert_equal "ysbaddaden/minitest.cr", spec.development_dependencies[0].url
       assert_equal "0.1.4", spec.development_dependencies[0].version
 
       assert_equal "webmock", spec.development_dependencies[1].name
-      assert_equal "git", spec.development_dependencies[1].resolver
+      assert_equal "git", spec.development_dependencies[1].resolver_name
       assert_equal "https://github.com/manastech/webcmok-crystal.git", spec.development_dependencies[1].url
       assert_equal "master", spec.development_dependencies[1].refs
     end
@@ -118,7 +160,6 @@ module Shards
           main: src/shards.cr
         cli:
           main: src/command/cli.cr
-          extra: foo
       YAML
 
       assert_equal 2, spec.targets.size
@@ -128,7 +169,19 @@ module Shards
 
       assert_equal "cli", spec.targets[1].name
       assert_equal "src/command/cli.cr", spec.targets[1].main
-      assert_equal "foo", spec.targets[1].attributes["extra"].to_s
+    end
+
+    def test_parse_target_missing_main
+      exc = assert_raises Shards::ParseError do
+        Spec.from_yaml <<-YAML
+          name: orm
+          version: 1.0.0
+          targets:
+            foo:
+              foo: bar
+          YAML
+      end
+      assert_equal %(Missing property "main" for target "foo" at line 4, column 3), exc.message
     end
 
     def test_parse_executables