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

Add support for extend self to handle_module_operation #2855

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

tlemburg
Copy link
Contributor

@tlemburg tlemburg commented Nov 14, 2024

By adding the instance methods as class methods through the singleton mixin operations, we are able to get all instance methods "copied" over into the class methods implicitly.

I don't think it's necessary to test whether this extend self applies when the module is opened after being closed again, or re-opened in another file, but we can add that as well if preferred.

Motivation

Closes #2782

Implementation

I simply allowed a SelfNode as an argument to enter the handle_module_operation method and then utilized
an Include entry to include the methods onto the singleton.

Automated Tests

Added tests checking that the method resolves as an instance method to the class as well as a class method. Further, we also test the inheritance tree for the singleton class.

Manual Tests

Tried a test file and ensured that methods were represented as class methods:

Screenshot 2024-11-14 at 3 50 52 PM

@tlemburg tlemburg requested a review from a team as a code owner November 14, 2024 22:00
@tlemburg tlemburg force-pushed the tlemburg/2782-support-extend-self branch from 82b49a9 to 23fc41b Compare November 14, 2024 22:08
@andyw8 andyw8 added enhancement New feature or request server This pull request should be included in the server gem's release notes labels Nov 18, 2024
@tlemburg tlemburg force-pushed the tlemburg/2782-support-extend-self branch from 23fc41b to 69634b6 Compare November 18, 2024 19:06
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

This solution is much better than what I had originally mentioned in the issue. The code looks good to me, let's just add one more assertion and we can ship.

lib/ruby_indexer/test/index_test.rb Show resolved Hide resolved
By adding the instance methods as class methods through the
singleton mixin operations, we are able to get all instance
methods "copied" over into the class methods implicitly.

I don't think it's necessary to test whether this extend self
applies when the module is opened after being closed again,
or re-opened in another file.

Closes Shopify#2782
@tlemburg tlemburg force-pushed the tlemburg/2782-support-extend-self branch from 69634b6 to 935eebd Compare November 19, 2024 15:00
@vinistock
Copy link
Member

Thanks for your contribution!

@vinistock vinistock merged commit a3ed380 into Shopify:main Nov 19, 2024
20 checks passed
@tlemburg tlemburg deleted the tlemburg/2782-support-extend-self branch November 19, 2024 16:11
@tlemburg
Copy link
Contributor Author

Thanks for your contribution!

You're welcome! Great you meet you and @andyw8 at the conf!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for extend self in the indexer
3 participants