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

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

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

vlad-pisanov
Copy link
Contributor

Currently, DefNode#void_context? incorrectly returns true for class methods called initialize. This PR fixes this.

# constructor: void context ✔️ 
def initialize
  42
end

# class method: not void context! ❌ 
def self.initialize
  42
end

@@ -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

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):

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.

@marcandre marcandre merged commit 0401cf2 into rubocop:master Sep 4, 2024
16 of 19 checks passed
@marcandre
Copy link
Contributor

Sorry for the delay, I missed the notification.

Thanks for the PR 👍

My cell phone just died on me, so I can't release right now, but will in a few days.

@koic
Copy link
Member

koic commented Sep 4, 2024

@marcandre If you’d like, I can go ahead and handle the release. Shall I handle the release?

@marcandre
Copy link
Contributor

Sure, that'd be great, thanks @koic ❤️

@koic
Copy link
Member

koic commented Sep 4, 2024

RuboCop AST 1.32.3 has been released. I hope your cell phone gets fixed soon :-)
https://rubygems.org/gems/rubocop-ast/versions/1.32.3

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.

4 participants