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

Inconsistent Hash Access #46

Open
krames opened this issue Jun 3, 2014 · 14 comments
Open

Inconsistent Hash Access #46

krames opened this issue Jun 3, 2014 · 14 comments
Labels

Comments

@krames
Copy link
Member

krames commented Jun 3, 2014

This issue was spurred by fog/fog#2939.

This is certainly not the first time, I have seen downstream developers expect to use string and symbols interchangeably. Doing a quick search through issues shows 33 issues have been submitted about this and something tells me there are a lot more that aren't being reported.

Should address this and if so how?

/cc: @geemus @tokengeek

@tokengeek
Copy link
Member

I'm not sure.

Part of me wonders if it is just bad Rails practice catching people out. Perhaps it's not our problem to solve.

Part of me wonders if it is more primitive obsession and is something that a real "Response", "Metadata" or "Credentials" object should be dealing with rather than souping up Hash.

I can see the benefits and the number of issues it has caused. Something is bugging me about just making it interchangeable though.

I'd rather us use a library like https://github.com/intridea/hashie than us roll our own though.

If this is such an obvious problem that Rails solved it why has it not made it into the Ruby language Hash itself?

Will someone bring out the old "Use Symbols because they are more efficient that Strings" blog post and then find a HTTP header you can not represent as a Symbol.

So I'm not sure either way. I'm pretty sure I've fallen for this problem myself (at least once today) but don't know if we want to be seeing people typecasting hashes everywhere to indifferent versions or monkey patching Hash itself.

Since the primary use case for fog is as a library for others, we shouldn't be monkey patching which will still allow people to pass in vanilla hashes and experience issues the same way.

We have a bit more control about returned Hashes though.

@krames
Copy link
Member Author

krames commented Jun 3, 2014

I have seen this issue in a lot of cases where people are using hashes as a parameter to the request layer and or passing in attribute information. (I know I am guilty of it)

My gut thought is that we should introduce some decorator or utility method to help fog devs address this concern. I can also see trying to standardize on strings or symbols. Something tells me both of those are easier said than done.

@nirvdrum
Copy link
Contributor

nirvdrum commented Jun 3, 2014

I've previously filed an issue. The real implementations all seem to interchangeably use Strings and Symbols, whereas all the mock implementations seem to only allow Strings. I think part of the historic oddity here is AWS uses headers that can't easily be represented as Symbols in Ruby 1.8. This might be cleared up in a 2.0 release if we standardized on Symbols everywhere shrug

@geemus
Copy link
Member

geemus commented Jun 3, 2014

Yeah, I also feel a bit dirty about instituting what seems like a Rails-ism. But I also want fog to be easy/usable. Standardizing on one or the other could also help, but as mentioned I'm fairly sure there would be exceptions still and confusion. I just feel so dirty about indifferent access...

@nirvdrum
Copy link
Contributor

nirvdrum commented Jun 3, 2014

To finish my half thought there, standardizing on Symbols in fog 2.0 might be easier as we go Ruby 1.9+.

@krames
Copy link
Member Author

krames commented Jun 3, 2014

I am indifferent (hah!) to which one we pick, but I am getting the feeling every one would prefer to standardize on symbols right?

@tokengeek Is this something that robocop could detect?

@nirvdrum
Copy link
Contributor

nirvdrum commented Jun 3, 2014

My preference is for symbols because of GC. But if you're using a lot of ad-hoc headers, then String have a benefit because they can be GC'd.

@tokengeek
Copy link
Member

@krames Not out of the box I don't think. You can write custom cops. So with that you could possibly pick up on warnings when strings are used as Hash keys. I'm not sure how much work it could be.

+1 for symbols though

@plribeiro3000
Copy link
Member

Same as fog/fog#362!

@cphrmky
Copy link
Member

cphrmky commented May 6, 2016

Was a consensus ever reached on this issue? I just ran into it with fog-softlayer/issues/86.

@nirvdrum
Copy link
Contributor

nirvdrum commented May 6, 2016

I'll just note that in more recent Ruby releases, symbols can be GC'd. With fog planning to go Ruby 2.0+, the difference in GC behavior for Strings and Symbols should fade.

(PS: I see that my previous comment was a bit ambiguous. My preference for symbols was to prevent overall object churn. Frozen string literals, however, basically mitigate that problem. My concern about symbols not being able to be GC'd was just that you could fill the symbol table with unbounded growth if arbitrary headers could be used. The symbol table can now GC, so that concern is also mitigated.)

@geemus
Copy link
Member

geemus commented May 10, 2016

Being consistent internally seems good, but being usable for end-users is perhaps even better/more important. I'm open to doing what we need to do and it sounds like we are moving toward a place where some of our more concrete concerns are taken care of (and whether we feel good about it or not perhaps doesn't matter as much as how usable it is or is not).

@cphrmky
Copy link
Member

cphrmky commented May 10, 2016

👍 for usability

@stale
Copy link

stale bot commented Jul 30, 2018

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 30, 2018
@geemus geemus added pinned and removed wontfix labels Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants