-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
[472] - Fix for Accept nullable parameter #498
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #498 +/- ##
==========================================
+ Coverage 90.08% 90.11% +0.03%
==========================================
Files 76 76
Lines 1584 1589 +5
==========================================
+ Hits 1427 1432 +5
Misses 157 157
☔ View full report in Codecov by Sentry. |
@bastelfreak - Can you review this, Please. |
@jahir-husain can you please rebase against our latest master branch and squash the commits down? |
valid_classes = TYPE_CLASS_MAPPINGS.fetch(type) { return true } | ||
valid_classes = [valid_classes, NilClass] if nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case when type is boolean, you need to flatten the valid_classes
type = "boolean"
valid_classes = TYPE_CLASS_MAPPINGS.fetch(type) # => [TrueClass, FalseClass]
valid_classes = [valid_classes, NilClass] if nullable # => [[TrueClass, FalseClass], NilClass]
Array(valid_classes).any? { |c| data.is_a?(c) } # => #<TypeError: class or module required>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking this @belinskidima
@jahir-husain can you please take a look at the comment and rebase against our latest master (so we get rid of the merge commits). |
@jahir-husain please rebase instead of doing a merge. |
any update? |
@kntmrkm this PR needs a rebase. If you want you can checkout the code locally, rebase it against our Head and then submit a new PR. |
|
Fix for issue #472