Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
amomchilov
changed the title
not_nil! assertion methodObject#not_nil! assertion
amomchilov
force-pushed
the
Alex/not_nil
branch
from
70be90d to
b24fc3a
Compare
dersam
left a comment
dersam
left a comment
There was a problem hiding this comment.
We copied this over to Shopify/roast a while ago. I'll swap over to this gem once it's open sourced.
cc: @juniper-shopify
| end | ||
|
|
||
| class NilClass | ||
| # @override |
There was a problem hiding this comment.
It's not strictly necessary. @override isn't unless the parent is explicitly marked @overrideable (whether you use RBS or sig {} syntax).
But this is indeed an override, since NilClass < Kernel, so I opted to document that fact
amomchilov
force-pushed
the
graphite-base/7
branch
from
d29b4c1 to
368b4ff
Compare
amomchilov
force-pushed
the
Alex/not_nil
branch
from
b24fc3a to
ef7aeed
Compare
amomchilov
force-pushed
the
Alex/not_nil
branch
from
ef7aeed to
c68741f
Compare
amomchilov
force-pushed
the
Alex/gem-infra
branch
from
368b4ff to
9bd8502
Compare
amomchilov
force-pushed
the
Alex/not_nil
branch
from
c68741f to
da8fd60
Compare
| module TypeToolkit | ||
| # This cop detects attempts to raise, rescue, or otherwise use the `UnexpectedNilError` class. | ||
| class DontExpectUnexpectedNil < Base | ||
| RESTRICT_ON_SEND = [:assert_raises, :raise].freeze |
There was a problem hiding this comment.
Do we still need raise here? Look like we commented the branch?
There was a problem hiding this comment.
The branch shouldn't be commented. And the tests should have caught that. 🤦♂️
There was a problem hiding this comment.
I was initially thinking following in the footsteps of Active Support, https://guides.rubyonrails.org/active_support_core_extensions.html#loading-all-active-support , but decided against and changed this in #13
| # so that bare rescues clauses (like `rescue => e`) don't rescue it. | ||
| class UnexpectedNilError < Exception # rubocop:disable Lint/InheritException | ||
| def initialize(message = "Called `not_nil!` on nil.") | ||
| super(message) |
There was a problem hiding this comment.
Can't is just be super?
| super(message) | |
| super |
| # `UnexpectedNilError` should never occur in well-formed code, so it should never be rescued. | ||
| # This is why it inherits from `Exception` instead of `StandardError`, | ||
| # so that bare rescues clauses (like `rescue => e`) don't rescue it. | ||
| class UnexpectedNilError < Exception # rubocop:disable Lint/InheritException |
There was a problem hiding this comment.
Do we want this to be namespaced or are we happy having it at the root?
There was a problem hiding this comment.
I'm leaning towards namespaced.
My dream is for this gem to be common/standard enough that it's worthy of owning top-level constants (to look "clean" and "native" 😎), but I won't get too big for my britches, so I'll make it namespaced.
4 participants
When debugging a nil-related error, it can be difficult to trace back where the
nilactually originated from. It could have come in from a parameter, whose argument was read from an instance variable, on an object loaded from a cache, populated by some totally different request.If a value can't be
nil, it's best for that to be clearly asserted as close to where that nilable value was first generated. That way, a roguenilisn't allowed to propagate arbitrary far away in downstream code.This PR:
Object#not_nil!method (inspired by https://bugs.ruby-lang.org/issues/17326 and https://github.com/ruby/ruby/pull/11772/files)