Conversation
spec/redis_session_store_spec.rb
Outdated
| end | ||
|
|
||
| it 'assigns the :ttl option to @default_options' do | ||
| expect(default_options[:ttl]).to eq(60 * 120) |
There was a problem hiding this comment.
maybe we can assign 60 * 120 to a variable and ref that here and on L72 so it's more obvious where it's coming from?
lib/redis-session-store.rb
Outdated
| alias write_session set_session | ||
|
|
||
| def get_expiry(env, options) | ||
| opts = options || env.fetch(ENV_SESSION_OPTIONS_KEY) |
There was a problem hiding this comment.
what about calling this expiry instead of opts ? That seems clear-er to me.
There was a problem hiding this comment.
it's not really expiry though -- its all the session storage options.
There was a problem hiding this comment.
oh you are right, I was misreading what was up there before. My basic feeling on this is that having options and opts vars in the same method is tricky. If we can find slightly better names for one or both, that would be 🆒
There was a problem hiding this comment.
how about redis_options instead of opts ?
pkarman
force-pushed
the
pek-alternate-expiration-config-key
branch
2 times, most recently
from
c8131fe to
101b938
Compare
|
|
lib/redis-session-store.rb
Outdated
| alias write_session set_session | ||
|
|
||
| def get_expiry(env, options) | ||
| session_storage_options = options || env.fetch(ENV_SESSION_OPTIONS_KEY) |
There was a problem hiding this comment.
Will ENV_SESSION_OPTIONS_KEY always be present? If not, then perhaps use fetch with a default value, such as env.fetch(ENV_SESSION_OPTIONS_KEY, {})? That way, if session_storage_options is nil, session_storage_options[:ttl] won't raise a NoMethodError.
There was a problem hiding this comment.
The existing code seems to assume that env.fetch(ENV_SESSION_OPTIONS_KEY) always exists, since I just cut/pasted that line. Happy to be a little defensive though.
|
Is it worth updating the README as well? |
pkarman
force-pushed
the
pek-alternate-expiration-config-key
branch
2 times, most recently
from
02a72cb to
bdd1b2b
Compare
|
@zachmargolis README updated. |
|
@monfresh @zachmargolis comments addressed, thanks. PTAL |
README.md
Outdated
| redis: { | ||
| expire_after: 120.minutes, | ||
| expire_after: 120.minutes, # cookie expiration | ||
| ttl: 120.minutes, # Redis expiration |
There was a problem hiding this comment.
"redis expiratoin, defaults to expire_after" ?
**Why**: The :expire_after config option controls both the Redis session expiration and the expiration of the Rails session cookie. Adding a new config option :ttl allows those controls to be managed separately. The most common case would be if you wanted the Rails cookie to expire when the browser is closed, as it would if :expire_after were set to `nil`.
pkarman
force-pushed
the
pek-alternate-expiration-config-key
branch
from
bdd1b2b to
71053a9
Compare
4 participants
Why: The :expire_after config option controls both the Redis
session expiration and the expiration of the Rails session cookie.
Adding a new config option :ttl allows those controls to be
managed separately.
The most common case would be if you wanted the Rails cookie
to expire when the browser is closed, as it would if :expire_after
were set to
nil.