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 private_class_method in the indexer #2858

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kcdragon
Copy link

Motivation

Closes #2781

private_class_method can be used to declare class methods as private. Prior to this PR, this method was not setting the visibility to private for the methods in the arguments.

Implementation

My implementation is similar to the module_function implementation which was mentioned in the issue.
I handled the case where the arguments are Strings, Symbols or an Array of those. They are mentioned as the valid arguments in the Ruby docs.

Automated Tests

I added tests to cover private_class_method being passed a combination or Strings and Symbols or an Array of Strings and Symbols.

Manual Tests

In this example,

class Test
  def self.foo
  end

  def self.bar
  end

  private_class_method(:foo)
end

foo does not get suggested but bar does.

Screenshot 2024-11-14 at 5 03 50 PM Screenshot 2024-11-14 at 5 04 00 PM

@kcdragon kcdragon requested a review from a team as a code owner November 14, 2024 23:05
@kcdragon
Copy link
Author

I have signed the CLA!

@andyw8
Copy link
Contributor

andyw8 commented Nov 15, 2024

Looks great!

Another way that private_class_method can be used is as a prefix to def, e.g.:

private_class_method def self.foo
  ...
end

Can we add a test to verify this doesn't cause anything to break?

entries = T.must(@index[keyword])
assert_equal(1, entries.size)
entry = entries.first
assert_equal(Entry::Visibility::PRIVATE, entry.visibility)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert_equal(Entry::Visibility::PRIVATE, entry.visibility)
assert_predicate(entry.visibility, :private?)

(I updated some other occurrences in #2859)

entries = T.must(@index[keyword])
assert_equal(1, entries.size)
entry = entries.first
assert_equal(Entry::Visibility::PRIVATE, entry.visibility)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert_equal(Entry::Visibility::PRIVATE, entry.visibility)
assert_predicate(entry.visibility, :private?)

@andyw8
Copy link
Contributor

andyw8 commented Nov 17, 2024

I pushed a commit to the branch below illustrating how an inline private_class_method def self.foo could be handled:

https://github.com/Shopify/ruby-lsp/tree/andyw8/def-private-class-method

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

I think we actually don't support the inline visibility thing even for other operations, like

private def foo; end

I'm good with moving forward with this PR and addressing that in a separate change.

@andyw8
Copy link
Contributor

andyw8 commented Nov 18, 2024

@vinistock we do:

# We want to restore the visibility stack when we leave a method definition with a visibility modifier
# e.g. `private def foo; end`
if node.arguments&.arguments&.first&.is_a?(Prism::DefNode)
@visibility_stack.pop
end

(and I verified manually)

@kcdragon
Copy link
Author

Thanks for the feedback! I'll update the PR over the next couple of days.

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 private_class_method in the indexer
3 participants