amberframework / amber

A Crystal web framework that makes building applications fast, simple, and enjoyable. Get started with quick prototyping, less bugs, and blazing fast performance.
https://amberframework.org
MIT License
2.57k stars 206 forks source link

Inconsistent Amber::Router::Session::AbstractStore API #1239

Closed bcardiff closed 3 years ago

bcardiff commented 3 years ago

The current state is the following:

abstract class AbstractStore
  abstract def update(other_hash)
end

class CookieStore < AbstractStore
  def update(hash : Hash(String | Symbol, String))
  end
end

class RedisStore < AbstractStore
  def update(hash : Hash(String, String))
  end
end

With 1.0 that will no longer compile due to https://github.com/crystal-lang/crystal/pull/9634.

What is the expected API? Hash(String | Symbol, String) or Hash(String, String) or any include Enumerable({K, String})? Whatever the choice it would be need to be reflected in the AbstractStore, or remove the type restrictions in the implementation classes letting the type error show in the call-site.

drujensen commented 3 years ago

@bcardiff we want to support (String | Symbol) as the key for the store. We might need to map the Symbol to a String if the Redis connection expects it here: https://github.com/amberframework/amber/blob/master/src/amber/router/session/redis_store.cr#L63