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

Silently attempt to fetch non-existent hash value #494

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timfjord
Copy link

When the array! method attempts to read a non-existent hash value it raises the KeyError error because it uses Hash#fetch method under the hood without the second argument.
This case is not covered with tests(the Hash case is not covered at all) so I assume it is not the expected behavior.
I cannot find any arguments when this behavior might be useful.

This PR fixes this issue and makes sure an attempt to read a non-existent hash value returns nil

@dhh
Copy link
Member

dhh commented Jan 22, 2021

Seems like its better to raise on a missing attribute than to silently return nil? Seems like a way you'd get screwed on spelling errors?

@timfjord
Copy link
Author

I'd consider that this piece of code

data = [{ id: 1, content: 'hello', meta: 'meta' }, { id: 2, content: 'world' }] 

json.array! data, :id, :content, :meta

is a shortcut for

data = [{ id: 1, content: 'hello', meta: 'meta' }, { id: 2, content: 'world' }] 

json.array! data do |hash|
  json.id hash[:id]
  json.content hash[:content]
  json.meta hash[:meta]
end

So I would expect them to behave similarly.

Of course we can say, that instead of Hash#[] we should have used Hash#fetch, plus, indeed, #fetch can help us with
spelling errors, so maybe we could handle that with an additional option?

json.array! data, :id, :content, :meta, fetch: true # assuming that `Hash#[]` is the default method

or

json.array! data, :id, :content, :meta, silent: true # assuming that `Hash#fetch` is the default method

@timfjord
Copy link
Author

timfjord commented Jan 22, 2021

Hm, we could even handle that completely differently and introduce default option so it can be used for both hashes and objects

json.array! data, :id, :content, :meta, default: ''

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.

2 participants