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

Support module_function in the indexer #2733

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

IgnacioFan
Copy link
Contributor

Motivation

Closes #2653

The module_function invocation creates both a singleton method and a private method when using include in a class. The indexer needs to account for this behavior by mutating the method's visibility and handling both method types.

Implementation

When the indexer encounters a module_function, it updates the corresponding method entry:

  • Changes the instance method’s visibility to private.
  • Adds the singleton method to the indexer (public).

Automated Tests

Add a new test

@IgnacioFan IgnacioFan requested a review from a team as a code owner October 17, 2024 14:25
@andyw8
Copy link
Contributor

andyw8 commented Oct 17, 2024

There are some typecheck failures.

@andyw8 andyw8 added the enhancement New feature or request label Oct 17, 2024
@IgnacioFan
Copy link
Contributor Author

There are some typecheck failures.

Hi @andyw8, Thank you for reviewing! I pushed a new commit, fixed the type check failures, and signed off on the CLA. Do I have to commit "I have signed the CLA!" to rerun the CI?

image

@vinistock vinistock added the server This pull request should be included in the server gem's release notes label Oct 18, 2024
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 is looking great!

@andyw8
Copy link
Contributor

andyw8 commented Oct 23, 2024

Let's also add a fixture for module_function for easier manual verification/testing?

(I was surprised to see that Prism doesn't have one already).

@vinistock
Copy link
Member

Let's also add a fixture for module_function for easier manual verification/testing?

Do we need one given that we have the unit test?

(I was surprised to see that Prism doesn't have one already).

module_function is just another Kernel method. There's nothing specific about parsing related to it because it's not a keyword.

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.

Thank you for the contribution!

@vinistock vinistock merged commit d21d40f into Shopify:main Oct 24, 2024
22 checks passed
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.

Support module_function in the indexer
3 participants