Improved settings and fixed tests (#364)

* Fixed session so that it could be tested without previous tests
effecting it's state randomly. Also conforms to best practices.

* resolved logging level settings with instance variables

* added settings to configure with block since it's probably clear than configure.

* added requested test change

spec/amber/amber_spec.cr

@@ -17,26 +17,8 @@ describe Amber do
 
   describe Amber::Server do
     describe ".configure" do
-      it "loads environment settings from test.yml" do
-        settings = Amber::Server.settings
-
-        settings.name.should eq "amber_test_app"
-        settings.port_reuse.should eq true
-        settings.redis_url.should eq "#{ENV["REDIS_URL"]? || "redis://localhost:6379"}"
-        settings.port.should eq 3000
-        settings.color.should eq true
-        settings.secret_key_base.should eq "mV6kTmG3k1yVFh-fPYpugSn0wbZveDvrvfQuv88DPF8"
-        # Sometimes settings get over written by other tests first and this fails
-        # expected_session = {:key => "amber.session", :store => "signed_cookie", :expires => "0"}
-        # settings.session.should eq expected_session
-        expected_secrets = {
-          description: "Store your test secrets credentials and settings here.",
-          database:    "mysql://root@localhost:3306/amber_test_app_test",
-        }
-        settings.secrets.should eq expected_secrets
-      end
-
       it "overrides enviroment settings" do
+        Amber::Server.instance.settings = Amber::Settings.new
         Amber::Server.configure do |server|
           server.name = "Hello World App"
           server.port = 8080
@@ -54,18 +36,22 @@ describe Amber do
       end
 
       it "retains environment.yml settings that haven't been overwritten" do
-        # NOTE: Any changes to settings here remain for all specs run afterwards.
-        # Added for convenience until settings is change to an instances
-        # instead of singleton this is still an "a problem".
+        Amber::Server.instance.settings = Amber::Settings.new
         Amber::Server.configure do |server|
-          server.name = "Hello World App"
+          server.name = "New name"
           server.port = 8080
         end
 
         settings = Amber::Server.settings
 
-        settings.name.should eq "Hello World App"
-        settings.port.should eq 8080
+        settings.name.should_not eq "Hello World App"
+        settings.port_reuse.should eq true
+        settings.redis_url.should eq "#{ENV["REDIS_URL"]? || "redis://localhost:6379"}"
+        settings.color.should eq true
+        settings.secret_key_base.should eq "mV6kTmG3k1yVFh-fPYpugSn0wbZveDvrvfQuv88DPF8"
+        expected_session = {:key => "amber.session", :store => :signed_cookie, :expires => 0}
+        settings.session.should eq expected_session
+        settings.port.should_not eq 3000
         expected_secrets = {
           description: "Store your test secrets credentials and settings here.",
           database:    "mysql://root@localhost:3306/amber_test_app_test",

spec/amber/router/session/redis_store_spec.cr

@@ -3,7 +3,7 @@ require "redis"
 
 # TODO: This test can't run on it's own because it needs the EXPIRES contant which is set elsewhere.
 module Amber::Router::Session
-  REDIS_STORE = Redis.new(url: Settings.redis_url)
+  REDIS_STORE = Redis.new(url: Amber::Server.redis_url)
 
   describe RedisStore do
     describe "#id" do

spec/amber/scripts/environment_loader_spec.cr

@@ -4,44 +4,42 @@ describe Amber::Server do
   describe "environment" do
     it "should load expected output" do
       expected = <<-EXP
-      @@name = "amber_test_app"
-      @@port_reuse = true
-      @@process_count = (ENV[%(AMBER_PROCESS_COUNT)]? || 1).to_i
-      @@log = ::Logger.new(STDOUT)
-      @@log.level = ::Logger::INFO
-      @@color = true
-      @@redis_url = "\#{ENV[%(REDIS_URL)]? || %(redis://localhost:6379)}"
-      @@port = 3000
-      @@host = "0.0.0.0"
-      @@secret_key_base = "mV6kTmG3k1yVFh-fPYpugSn0wbZveDvrvfQuv88DPF8"
-      @@session = {
+      @name = "amber_test_app"
+      @port_reuse = true
+      @process_count = (ENV[%(AMBER_PROCESS_COUNT)]? || 1).to_i
+      @log = ::Logger.new(STDOUT).tap{|l| l.level = ::Logger::INFO}
+      @color = true
+      @redis_url = "\#{ENV[%(REDIS_URL)]? || %(redis://localhost:6379)}"
+      @port = 3000
+      @host = "0.0.0.0"
+      @secret_key_base = "mV6kTmG3k1yVFh-fPYpugSn0wbZveDvrvfQuv88DPF8"
+      @session = {
         :key => "amber.session",
         :store => :signed_cookie,
         :expires => 0, 
       }
-      class_getter secrets = {"description": "Store your test secrets credentials and settings here.", "database": "mysql://root@localhost:3306/amber_test_app_test"}
+      getter secrets = {"description": "Store your test secrets credentials and settings here.", "database": "mysql://root@localhost:3306/amber_test_app_test"}
 
       EXP
       {{run("../../../src/amber/scripts/environment_loader.cr", "test").stringify}}.should eq expected
     end
 
     it "should load default settings when environment file doesn't exist." do
       expected = <<-EXP
-      @@name = "Amber_App"
-      @@port_reuse = true
-      @@process_count = 1
-      @@log = ::Logger.new(STDOUT)
-      @@log.level = ::Logger::INFO
-      @@color = true
-      @@redis_url = "redis://localhost:6379"
-      @@port = 3000
-      @@host = "127.0.0.1"
-      @@session = {:key => "amber.session", :store => :signed_cookie, :expires => 0}
-      class_getter secrets = {description: "Store your non_existent secrets credentials and settings here."}
+      @name = "Amber_App"
+      @port_reuse = true
+      @process_count = 1
+      @log = ::Logger.new(STDOUT).tap{|l| l.level = ::Logger::INFO}
+      @color = true
+      @redis_url = "redis://localhost:6379"
+      @port = 3000
+      @host = "127.0.0.1"
+      @session = {:key => "amber.session", :store => :signed_cookie, :expires => 0}
+      getter secrets = {description: "Store your non_existent secrets credentials and settings here."}
 
       EXP
       # Removed secret_key_base from default settings since it's different everytime.
-      {{run("../../../src/amber/scripts/environment_loader.cr", "non_existent").stringify}}.gsub(/@@secret_key_base[^\n]+\n/, "").should eq expected
+      {{run("../../../src/amber/scripts/environment_loader.cr", "non_existent").stringify}}.gsub(/@secret_key_base[^\n]+\n/, "").should eq expected
     end
   end
 end

spec/amber/server/settings_spec.cr

@@ -0,0 +1,21 @@
+require "../../../spec_helper"
+
+describe Amber::Settings do
+  it "loads environment settings from test.yml" do
+    settings = Amber::Settings.new
+
+    settings.name.should eq "amber_test_app"
+    settings.port_reuse.should eq true
+    settings.redis_url.should eq "#{ENV["REDIS_URL"]? || "redis://localhost:6379"}"
+    settings.port.should eq 3000
+    settings.color.should eq true
+    settings.secret_key_base.should eq "mV6kTmG3k1yVFh-fPYpugSn0wbZveDvrvfQuv88DPF8"
+    expected_session = {:key => "amber.session", :store => :signed_cookie, :expires => 0}
+    settings.session.should eq expected_session
+    expected_secrets = {
+      description: "Store your test secrets credentials and settings here.",
+      database:    "mysql://root@localhost:3306/amber_test_app_test",
+    }
+    settings.secrets.should eq expected_secrets
+  end
+end

src/amber.cr

@@ -76,11 +76,11 @@ module Amber
     end
 
     def colorize(text, color)
-      text.colorize(color).toggle(Amber::Settings.color).to_s
+      text.colorize(color).toggle(settings.color).to_s
     end
 
     def colorize(text, color, mode)
-      text.colorize(color).toggle(Amber::Settings.color).mode(mode).to_s
+      text.colorize(color).toggle(settings.color).mode(mode).to_s
     end
   end
 end

src/amber/router/pipe/logger.cr

@@ -4,7 +4,7 @@ module Amber
   module Pipe
     class Logger < Base
       def colorize(text, color)
-        text.colorize(color).toggle(Amber::Settings.color).to_s
+        text.colorize(color).toggle(Amber::Server.settings.color).to_s
       end
 
       def initialize(io : IO = STDOUT)

src/amber/scripts/environment_loader.cr

@@ -34,41 +34,41 @@ str = String.build do |s|
   # Most of this logic can be cleaned up by just requiring environment files to contain valid params.
   # For now I have this here so that tests can still pass without having to load env files although they should.
   # This is a transistion.
-  s.puts %(@@name = "#{settings["name"]? || "Amber_App"}")
-  s.puts %(@@port_reuse = #{settings["port_reuse"]? == nil ? true : settings["port_reuse"]})
-  s.puts %(@@process_count = #{settings["process_count"]? || 1})
-  s.puts %(@@log = #{settings["log"]? || "::Logger.new(STDOUT)"})
-  s.puts %(@@log.level = #{settings["log_level"]? || "::Logger::INFO"})
-  s.puts %(@@color = #{settings["color"]? == nil ? true : settings["color"]})
-  s.puts %(@@redis_url = "#{settings["redis_url"]? || "redis://localhost:6379"}")
-  s.puts %(@@port = #{settings["port"]? || 3000})
-  s.puts %(@@host = "#{settings["host"]? || "127.0.0.1"}")
-  s.puts %(@@secret_key_base = "#{settings["secret_key_base"]? || SecureRandom.urlsafe_base64(32)}")
+  s.puts %(@name = "#{settings["name"]? || "Amber_App"}")
+  s.puts %(@port_reuse = #{settings["port_reuse"]? == nil ? true : settings["port_reuse"]})
+  s.puts %(@process_count = #{settings["process_count"]? || 1})
+  s.puts %(@log = #{settings["log"]? || "::Logger.new(STDOUT)"}.tap{|l| l.level = #{settings["log_level"]? || "::Logger::INFO"}})
+  #s.puts %(@log.level = #{settings["log_level"]? || "::Logger::INFO"})
+  s.puts %(@color = #{settings["color"]? == nil ? true : settings["color"]})
+  s.puts %(@redis_url = "#{settings["redis_url"]? || "redis://localhost:6379"}")
+  s.puts %(@port = #{settings["port"]? || 3000})
+  s.puts %(@host = "#{settings["host"]? || "127.0.0.1"}")
+  s.puts %(@secret_key_base = "#{settings["secret_key_base"]? || SecureRandom.urlsafe_base64(32)}")
 
   unless settings["ssl_key_file"]?.to_s.empty?
-    s.puts %(@@ssl_key_file = "#{settings["ssl_key_file"]?}")
+    s.puts %(@ssl_key_file = "#{settings["ssl_key_file"]?}")
   end
 
   unless settings["ssl_cert_file"]?.to_s.empty?
-    s.puts %(@@ssl_cert_file = "#{settings["ssl_cert_file"]?}")
+    s.puts %(@ssl_cert_file = "#{settings["ssl_cert_file"]?}")
   end
 
   if settings["session"]? && settings["session"].raw.is_a?(Hash(YAML::Type, YAML::Type))
     s.puts <<-SESSION
-    @@session = {
+    @session = {
       :key => "#{settings["session"]["key"]? ? settings["session"]["key"] : "amber.session"}",
       :store => #{settings["session"]["store"]? ? settings["session"]["store"] : ":signed_cookie"},
       :expires => #{settings["session"]["expires"]? ? settings["session"]["expires"] : 0}, 
     }
     SESSION
   else
-    s.puts %(@@session = {:key => "amber.session", :store => :signed_cookie, :expires => 0})
+    s.puts %(@session = {:key => "amber.session", :store => :signed_cookie, :expires => 0})
   end
 
   if settings["secrets"]? && settings["secrets"].raw.is_a?(Hash(YAML::Type, YAML::Type))
-    s.puts "class_getter secrets = #{settings["secrets"].inspect.gsub(/(\"[^\"]+\") \=\>/) { "#{$1}:" }}"
+    s.puts "getter secrets = #{settings["secrets"].inspect.gsub(/(\"[^\"]+\") \=\>/) { "#{$1}:" }}"
   else
-    s.puts %(class_getter secrets = {description: "Store your #{environment} secrets credentials and settings here."})
+    s.puts %(getter secrets = {description: "Store your #{environment} secrets credentials and settings here."})
   end
 end
 

src/amber/server/configuration.cr

@@ -1,20 +1,23 @@
 module Amber
   module Configuration
     macro included
+      property settings = Settings.new
+
       def self.instance
         @@instance ||= new
       end
-
+
+      # Configure should probably be deprecated in favor of settings.
       def self.configure
         with settings yield settings
       end
 
-      def self.settings
-        Settings
+      def self.settings(&block)
+        with settings yield settings
       end
 
-      def settings
-        Settings
+      def self.settings
+        instance.settings
       end
 
       def self.secret_key_base

src/amber/server/settings.cr

@@ -6,21 +6,21 @@ module Amber
   class Settings
     include Amber::DSL::Server
     alias WebSocketAdapter = WebSockets::Adapters::RedisAdapter.class | WebSockets::Adapters::MemoryAdapter.class
-    class_getter handler = Pipe::Pipeline.new
-    class_getter router = Router::Router.new
-    class_property pubsub_adapter : WebSocketAdapter = WebSockets::Adapters::MemoryAdapter
-    class_property port_reuse : Bool
-    class_property log : ::Logger
-    class_property color : Bool = true
-    class_property process_count : Int32
-    class_property secret_key_base : String
-    class_property port : Int32
-    class_property name : String
-    class_property host : String
-    class_property ssl_key_file : String? = nil
-    class_property ssl_cert_file : String? = nil
-    class_property redis_url = ""
-    class_property session : Hash(Symbol, Symbol | String | Int32)
+    getter handler = Pipe::Pipeline.new
+    getter router = Router::Router.new
+    property pubsub_adapter : WebSocketAdapter = WebSockets::Adapters::MemoryAdapter
+    property port_reuse : Bool
+    property log : ::Logger
+    property color : Bool = true
+    property process_count : Int32
+    property secret_key_base : String
+    property port : Int32
+    property name : String
+    property host : String
+    property ssl_key_file : String? = nil
+    property ssl_cert_file : String? = nil
+    property redis_url = ""
+    property session : Hash(Symbol, Symbol | String | Int32)
 
     # Loads environment yml settings from the current AMBER_ENV environment variable
     # and defaults to development environment