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.

Sign up for GitHub

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

Jump to bottom

Sinatra::IndifferentHash does not support initialization with any hash or key-value pair list #1948

Closed
nelsnelson opened this issue Sep 19, 2023 · 9 comments · Fixed by #1951
Closed

Sinatra::IndifferentHash does not support initialization with any hash or key-value pair list #1948

nelsnelson opened this issue Sep 19, 2023 · 9 comments · Fixed by #1951
Labels
docs

Comments

@nelsnelson
Copy link

In short, the following snippet from https://api.rubyonrails.org/classes/ActiveSupport/HashWithIndifferentAccess.html does not work:

hash = ActiveSupport::HashWithIndifferentAccess.new(a: 1) # => {"a"=>1}
hash.keys # => ["a"]

Instead, this is what happens with Sinatra::IndifferentHash:

hash = Sinatra::IndifferentHash.new(a: 1) # => {}
hash.keys # => []
@dentarg
Copy link
Member

dentarg commented Sep 19, 2023

This looks like an intentional omission by 44c3cb8 (#1279) (6 years ago), as this was possible with ActiveSupport::HashWithIndifferentAccess 14 years ago: rails/rails@0bd668f

Also:

$ irb
irb(main):001:0> Hash.new(a: 1)
=> {}

Maybe @mwpastore or @jkowens want to weigh in

@dentarg
Copy link
Member

dentarg commented Sep 19, 2023

Though the documentation comments should probably be updated:

sinatra/lib/sinatra/indifferent_hash.rb

Lines 22 to 32 in e130771

# belongs to the public interface. For example, given:
#
# hash = Sinatra::IndifferentHash.new(:a=>1)
#
# You are guaranteed that the key is returned as a string:
#
# hash.keys # => ["a"]
#
# Technically other types of keys are accepted:
#
# hash = Sinatra::IndifferentHash.new(:a=>1)

Looks like they were just copied and some search-and-replace was done

@nelsnelson
Copy link
Author

Interesting that it was an intentional omission. Thanks for the information, @dentarg . 🙇

Is there a known explanation for why the current active_support version's support for initializing with a constructor hash might not have been ported?

I guess if its not a bug, do you think a feature addition proposal has a chance of acceptance?

@dentarg
Copy link
Member

dentarg commented Sep 20, 2023 •
edited
Loading

I'm leaning towards that we shouldn't add that to Sinatra, as less code is less code to maintain, and the Sinatra project is very short on maintainers and maintainers time. Sinatra is not Rails/ActiveSupport so we shouldn't need to mimic every aspect of it.

I'm open to hear thoughts from the rest of @sinatra/sinatras-helpers though

@dentarg
Copy link
Member

dentarg commented Sep 20, 2023

Instead, time could be better spent on allowing users of Sinatra to use ActiveSupport::HashWithIndifferentAccess over Sinatra::IndifferentHash if they want (the user would need to add active_support as a dependency to their project). I don't know the feasibility of this idea though.

@nelsnelson
Copy link
Author

@dentarg - Those are good recommendations. For myself personally, I don't have any trouble using ActiveSupport instead, since it's required by another project dependency already.

I also have a work-around for the unsupported IndifferentHash.new(hash) usage, but it's cumbersome, and involves creating an empty IndifferentHash.new and assigning each key-value entry from hash to the indifferent one. It's pretty terrible.

I suppose what I'm really asking is, how difficult generally is contribution to Sinatra? I enjoy writing ruby code, and I'd be happy to commit some personal time to it. What is your opinion on such a contribution proposal? I find myself wanting to use IndifferentHash quite often. It'd be nice to make it easier to use.

@jkowens
Copy link
Member

jkowens commented Sep 22, 2023

Currently Sinatra::IndifferentHash.new behaves more like a real Hash where the object passed to new becomes the default value for missing keys.

ex:

 h = Sinatra::IndifferentHash.new(a: 1)
 h #=> {}
 h[:b] #=> {"a"=>1}

The documentation definitely should be updated to make that clear.

@nelsnelson
Copy link
Author

I see. Well, I suppose I'll close this issue, then. Thanks for the discussion, everyone.

@nelsnelson nelsnelson closed this as not planned Won't fix, can't repro, duplicate, stale Sep 23, 2023
@dentarg
Copy link
Member

dentarg commented Sep 23, 2023

Let's keep it open until the documentation has been updated, so we don't forget.

@dentarg dentarg reopened this Sep 23, 2023
@jkowens jkowens mentioned this issue Oct 8, 2023
Merged
jkowens added a commit that referenced this issue Oct 10, 2023
@jkowens @dentarg
Fix incorrect documentation for Sinatra::IndifferentHash (#1951)
6ddd6b6 
Resolves #1948. Looking over the code, I'm not sure that the
`initialize` method makes sense to me.

```
    def initialize(*args)
      args.map!(&method(:convert_value))
      super(*args)
    end
 ```
 
 I don't know why you'd want to call `convert_value` on a value that's been specified as a default value when a key is missing. See https://ruby-doc.org/3.2.2/Hash.html#method-c-new


```
h = Sinatra::IndifferentHash.new(a: 1)
h[:b] = {"a" => 1 }
```

I'm thinking maybe that should be removed to eliminate possible confusion?

Co-authored-by: Patrik Ragnarsson <patrik@starkast.net>
@mwpastore mwpastore mentioned this issue Oct 18, 2023
Closed
dentarg added a commit to dentarg/sinatra that referenced this issue Jan 5, 2024
@dentarg
Remove the IndifferentHash initializer
4f27831 
See these links for background:
- sinatra#1951
- sinatra#1948

Before

    > Sinatra::IndifferentHash.new(a: 1)[:b]
    => {"a"=>1}

After

    > Sinatra::IndifferentHash.new(a: 1)[:b]
    => {:a=>1}

Close sinatra#1953
@dentarg dentarg mentioned this issue Jan 5, 2024
Merged
dentarg added a commit that referenced this issue Jan 5, 2024
@dentarg
Remove the IndifferentHash initializer (#1982)
ceb14ed 
See these links for background:
- #1951
- #1948

Before

    > Sinatra::IndifferentHash.new(a: 1)[:b]
    => {"a"=>1}

After

    > Sinatra::IndifferentHash.new(a: 1)[:b]
    => {:a=>1}

Close #1953
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix incorrect documentation for Sinatra::IndifferentHash sinatra/sinatra
3 participants
@nelsnelson @dentarg @jkowens