Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add session#fetch #12692

Merged
merged 1 commit into from Oct 29, 2013
Merged

Add session#fetch #12692

merged 1 commit into from Oct 29, 2013

Conversation

@dmathieu
Copy link
Contributor

@dmathieu dmathieu commented Oct 29, 2013

This adds a #fetch method to the session object.

session.fetch(:value, 'default')

I've made this fetch behave like Hash#fetch, which means it takes either a default value, a block or no value.

@@ -127,6 +127,18 @@ def delete(key)
@delegate.delete key.to_s
end

def fetch(key, default=nil)

This comment has been minimized.

@senny

senny Oct 29, 2013
Member

Let's also add some rDoc to explain the usage and maybe refer to Hash#fetch

This comment has been minimized.

@senny

senny Oct 29, 2013
Member

just saw that saw that Session is :nodoc:... wondering why that is...

This comment has been minimized.

@dmathieu

dmathieu Oct 29, 2013
Author Contributor

I would definitely do that.
But since no other method in this class has documentation, I would think it might not be the best place.

This comment has been minimized.

@senny

senny Oct 29, 2013
Member

It looks like all the rDoc related to sessions is currently stored in base.rb. @fxn where should such methods be documented? As Session is :nodoc it looks like it's an implementation detail and not part of the public API. It's a bit sad that this way you won't be able to find the docs using the "Search" feature of api.rubyonrails.org:

bildschirmfoto 2013-10-29 um 20 35 25

@senny
senny reviewed Oct 29, 2013
View changes
actionpack/test/dispatch/request/session_test.rb Outdated
@@ -61,6 +61,20 @@ def test_clear
assert_equal([], s.values)
end

def test_fetch
env = {}

This comment has been minimized.

@senny

senny Oct 29, 2013
Member

is there a reason to have the env variable or could we do:

Session.create(store, {}, {})

This comment has been minimized.

@dmathieu

dmathieu Oct 29, 2013
Author Contributor

We could avoid having the env variable. I did it like this because all the other tests are formatted the same way.

This comment has been minimized.

@senny

senny Oct 29, 2013
Member

I see. The early ones also made use of that variable. To me it's confusing. When I look at the assignment I assume that env plays a special role but it's not relevant for that case.

This comment has been minimized.

@dmathieu

dmathieu Oct 29, 2013
Author Contributor

All right. I just removed it.

@carlosantoniodasilva
carlosantoniodasilva reviewed Oct 29, 2013
View changes
actionpack/CHANGELOG.md Outdated
@@ -1,3 +1,15 @@
* Add `session#fetch` method

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Oct 29, 2013
Member

Mistakenly indented

@guilleiguaran
Copy link
Member

@guilleiguaran guilleiguaran commented Oct 29, 2013

Looks fine for me 👍

iirc, some core members are a bit skeptic about add new methods to params/session hashes but this shouldn't be te case since we have fetch already in params 😄

guilleiguaran added a commit that referenced this pull request Oct 29, 2013
Add session#fetch
@guilleiguaran guilleiguaran merged commit a5e2800 into rails:master Oct 29, 2013
@dmathieu dmathieu deleted the dmathieu:session_fetch branch Oct 29, 2013
@splattael
Copy link
Contributor

@splattael splattael commented Oct 30, 2013

AFAIK, Hash#fetch does not modify its state when invoked with a default value or a block.

>> h = {}
=> {}
>> h.fetch(23) { :yo }
=> :yo
>> h.fetch(23)
KeyError: key not found: 23
    from (irb):25:in `fetch'
    from (irb):25
    from /home/ps/.rvm/rubies/ruby-2.0.0-p195/bin/irb:16:in `<main>'

Fetching a missing key in ActionDispatch::Request::Session modifies its state:

>> s = ActionDispatch::Request::Session.create(store, {}, {})
=> #<ActionDispatch::Request::Session:0x1976b88 not yet loaded>
>> s.fetch(23) { :yo }
=> :yo
>> s.fetch(23)
=> :yo

The second call should raise a KeyError.

The implementation of Session#fetch must not store the default value or the result of block.

@dmathieu
Copy link
Contributor Author

@dmathieu dmathieu commented Oct 30, 2013

Actually, I did this on purpose, as I would think this could lead to unexpected behaviors.

session.fetch(:read_articles, {})[21] = true

In this case, the saved value would be expected to be saved in the session.
There's no point in just having an anonymous hash object.

I agree I missed a test for this though. I can happily add it, or change the current behavior.

@rrrene
Copy link

@rrrene rrrene commented Oct 30, 2013

I'm with @splattael on this one. The behaviour you implement might have very valid use cases (but the example you give here isn't one of them and seems artificial). But it is not okay if the feature is advertised as "I've made this fetch behave like Hash#fetch" and then it doesn't live up to this promise (and moreover has serious side-effects).

On a side note, I strongly disagree with this opinion:

There's no point in just having an anonymous hash object.

This is false on so many levels 😔

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Oct 30, 2013

@dmathieu I agree with both about the different behavior between Hash#fetch and Session#fetch, but I also agree this is a nice thing. So I suggest to remove the sentence "fetch behaves like Hash#fetch." from changelog since it doesn't behave.

@dmathieu
Copy link
Contributor Author

@dmathieu dmathieu commented Oct 30, 2013

@splattael, @rrrene: I just pushed a fix to the concern you raised, which has just been merged. Thanks @rafaelfranca 💛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.