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.

Sign up for GitHub

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

Jump to bottom

Fix RuboCop::AST::DefNode#void_context? to handle class methods called initialize #310

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Fix RuboCop::AST::DefNode#void_context? to handle class methods called initialize #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/fix_def_node_void_context.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#310](https://github.com/rubocop/rubocop-ast/pull/310): Fix `RuboCop::AST::DefNode#void_context?` to handle class methods called `initialize`. ([@vlad-pisanov][])
2 changes: 1 addition & 1 deletion lib/rubocop/ast/node/def_node.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class DefNode < Node
#
# @return [Boolean] whether the `def` node body is a void context
def void_context?
method?(:initialize) || assignment_method?
(def_type? && method?(:initialize)) || assignment_method?
Copy link
Contributor

@Earlopain Earlopain Aug 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually this makes sense to me, though I wonder if would be better to instead introduce a DefsNode instead which then overrides this method. Not sure if worth the effort, lets wait on other input

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually thought DefsNode was already defined going into this.

I'd rather not make drastic changes in this PR. This is a pre-existing pattern (used in BlockNode for example):

rubocop-ast/lib/rubocop/ast/node/block_node.rb

Line 49 in 30439db

if numblock_type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. Yeah, probably fine then. I did a quick search around but didn't find other nodes that did it like this but clearly I didn't search good enough.

end

# Checks whether this method definition node forwards its arguments
Expand Down
6 changes: 6 additions & 0 deletions spec/rubocop/ast/def_node_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,12 @@
it { is_expected.to be_void_context }
end

context 'with a class method called "initialize"' do
let(:source) { 'def self.initialize(bar); end' }

it { is_expected.not_to be_void_context }
end

context 'with a regular assignment method' do
let(:source) { 'def foo=(bar); end' }

Expand Down
Loading