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

convert to sym value from session #21 #23

Merged
merged 5 commits into from
Dec 21, 2015
Merged

Conversation

afdev82
Copy link
Contributor

@afdev82 afdev82 commented Dec 17, 2015

In this way we are sure that we obtain always a symbol from the cookie.
I don't know how to change the test, I thought we could set the cookie to a valid string value ('world') instead of :default

@n0nick
Copy link
Member

n0nick commented Dec 18, 2015

  1. I'm not sure why casting to a symbol is necessary - could you give me an example for how this affects your use-case and/or what you would like to do with symbols?
  2. The tests fail because you are introducing a dependency on ActiveSupport's #try, but GeoRedirect does not assume the availability of ActiveSupport (i.e. for non-Rails Rack apps, etc). So, please change your logic to cast to a symbol without using try.
  3. After that, the tests should behave the same, but I would introduce another tests to make sure that values are always returned as symbols.

@afdev82
Copy link
Contributor Author

afdev82 commented Dec 18, 2015

Hi, thanks for the reply.
Yes, I will change to not use "try" for the conversion.
I use the gem in a Rails 4.2+ app and it seems that Rails casts the value in the cookie to be "string" instead of symbol.
When the code checks if the config contains the host that we have in the session (see here), it will return always false, because it checks against symbols, but in the variable we have always (in my case) a string.
I thought the best thing (if you want to use symbols as keys in the configuration file) is to convert always to symbols.

@n0nick
Copy link
Member

n0nick commented Dec 18, 2015

OK, that's a good-enough reason to cast. Although I must say, I'm using the gem in a Rails 4.2.3 app and have not experienced such issue.

@afdev82
Copy link
Contributor Author

afdev82 commented Dec 18, 2015

Strange, but I could investigate further. The version I'm using is the 4.2.5

@n0nick
Copy link
Member

n0nick commented Dec 18, 2015

I'll happily merge a patch that fixes the session values casting to symbols.
If that solves your issue, it might point to one of many possible differences between our apps :)

@n0nick
Copy link
Member

n0nick commented Dec 21, 2015

well, cloning, bundle install and bundle exec rspec spec should do the trick.

@afdev82
Copy link
Contributor Author

afdev82 commented Dec 21, 2015

Thanks ;)

n0nick added a commit that referenced this pull request Dec 21, 2015
Host value: Always cast to symbol
@n0nick n0nick merged commit 9d67410 into wazeHQ:master Dec 21, 2015
@n0nick
Copy link
Member

n0nick commented Dec 21, 2015

and now it's up in version 0.5.1, thanks! :)

@afdev82 afdev82 deleted the fix_use_session branch December 21, 2015 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants