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

Optionally raise on empty hash filter definitions #158

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mariokostelac
Copy link

I've recently spent several hours debugging a problem that was caused by an empty hash definition

required do
   hash :hash_name
end

Such definition discards any hash content being passed in, which is very
confusing and I reckon rarely useful, but very unintuitive interface
(found other 3 bugs in the codebase caused by the exact same problem).

This change makes such definitions raise, but only if
Mutations.raise_on_empty_hash_filter is set to true (defualt false).
Such optional raising avoids breaking change.

Overall, I recommend changing the interface to raise on bogus definition
as a default, instead of the opt-in.

I've recently spent several hours debugging a problem that was caused by an empty hash definition
```ruby
required do
   hash :hash_name
end
```

Such definition discards any hash content being passed in, which is very
confusing and I reckon rarely useful, but very unintuitive interface
(found other 3 bugs in the codebase caused by the exact same problem).

This change makes such definitions raise, but only if
Mutations.raise_on_empty_hash_filter is set to true (defualt false).
Such optional raising avoids breaking change.

Overall, I recommend changing the interface to raise on bogus definition
as a default, instead of the opt-in.
@mariokostelac mariokostelac force-pushed the mario/raise_on_empty_hash branch from f81a8e9 to d245226 Compare August 5, 2021 07:17
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.

1 participant