-
Notifications
You must be signed in to change notification settings - Fork 0
Move UnexpectedNilError into gem namespace
#16
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,20 @@ | ||
| # typed: true | ||
| # frozen_string_literal: true | ||
|
|
||
| require "type_toolkit/ext/nil_assertions" | ||
|
|
||
| module RuboCop | ||
| module Cop | ||
| 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 | ||
|
|
||
| UNEXPECTED_NIL_ERROR_NAME = ::TypeToolkit::UnexpectedNilError.name.not_nil! | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's intentional duplication, to make sure this is working correctly. |
||
| .split("::").last.not_nil! | ||
| .to_sym #: Symbol | ||
| private_constant :UNEXPECTED_NIL_ERROR_NAME | ||
|
Comment on lines
+13
to
+16
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Un-hard-coding this, to make sure it stays in sync with the actual constant we care about. |
||
|
|
||
| #: (RuboCop::AST::SendNode) -> void | ||
| def on_send(node) | ||
| case node.method_name | ||
|
|
@@ -55,7 +62,12 @@ def on_investigation_end | |
|
|
||
| #: (RuboCop::AST::ConstNode) -> bool | ||
| def unexpected_nil_error?(node) | ||
| node.short_name == :UnexpectedNilError && (node.namespace.nil? || node.namespace.cbase_type?) | ||
| return false unless node.short_name == UNEXPECTED_NIL_ERROR_NAME | ||
|
|
||
| ns = node.namespace | ||
| return true if ns.nil? || ns.cbase_type? | ||
|
|
||
| ns.const_type? && ns.short_name == :TypeToolkit && (ns.namespace.nil? || ns.namespace.cbase_type?) | ||
| end | ||
|
|
||
| # Check for `raise UnexpectedNilError` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,17 +18,19 @@ class NilClass | |
| # @override | ||
| #: -> bot | ||
| def not_nil! | ||
| raise UnexpectedNilError | ||
| raise TypeToolkit::UnexpectedNilError | ||
| end | ||
| end | ||
|
|
||
| # An error raised when calling `#not_nil!` on a `nil` value. | ||
| # | ||
| # `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 | ||
| def initialize(message = "Called `not_nil!` on nil.") | ||
| super(message) | ||
| module TypeToolkit | ||
| # An error raised when calling `#not_nil!` on a `nil` value. | ||
| # | ||
| # `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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's now
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. The tests got really duplicative, so I parameterized them. See the two commits separately There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh nice, tests are much cleaner now. LGTM, there are some type checking errors though.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed! |
||
| def initialize(message = "Called `not_nil!` on nil.") | ||
| super | ||
| end | ||
| end | ||
| end | ||
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.
Quick question: does loading this cop now patch
NilClassandKernelas a side effect? Users who configure the cop without wanting the runtime extensions would get them implicitly.Was this the intended design, or should the constant be defined another way to avoid that side effect?
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.
Yes it does,
and yeah I could extract out the error class to its own file, but I'll cross that bridge if someone has a really good reason to want that. Was hoping to be opinionated/KISS for now