Account for private constants in features #1004
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vinistock
force-pushed
the
vs/handle_private_constant
branch
from
e69179d to
eae8b7b
Compare
vinistock
force-pushed
the
vs/account_for_private_constants_in_features
branch
from
97051dc to
0a1d9fd
Compare
vinistock
force-pushed
the
vs/account_for_private_constants_in_features
branch
from
0a1d9fd to
fad6231
Compare
andyw8
reviewed
Sep 20, 2023
| end | ||
|
|
||
| def test_completion_private_constants_from_different_namespace | ||
| RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false) |
There was a problem hiding this comment.
Not in this PR, but since we use this a lot I wonder we should extract it out, e.g.:
def with_typechecker_disabled(&block)
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, false)
yield
ensure
RubyLsp::DependencyDetector.const_set(:HAS_TYPECHECKER, true)
endThere was a problem hiding this comment.
That's true. We can extract it in another PR.
andyw8
reviewed
Sep 20, 2023
| RUBY | ||
|
|
||
| result = RubyLsp::Requests::WorkspaceSymbol.new("Foo::CONSTANT", @index).run | ||
| assert_equal(1, result.length) |
There was a problem hiding this comment.
I may be misunderstanding the test, but if we're looking up Foo::CONSTANT then why is Foo in the results?
There was a problem hiding this comment.
Because we fuzzy match the results for workspace symbol and the Jaro-Winkler similarity between Foo and Foo::CONSTANT is high. The reason is likely because Jaro-Winkler gives a higher importance to the prefix than it does to the rest of the word
ruby-lsp(main):001> DidYouMean::JaroWinkler.distance("Foo", "Foo::CONSTANT")
=> 0.8205128205128205
There was a problem hiding this comment.
Ah, I see. That may be worth a code comment.
andyw8
approved these changes
Sep 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Closes #966
We should only provide definition, completion and hover for private constants if the reference happens inside the exact same namespace as they are defined. Otherwise, it's a reference to a constant that doesn't actually exist.
Similarly, private constants should never be shown in workspace symbol.
Implementation
I split the PR by commits to make it easier to review.
On each request, started skipping adding results if we find a non-private reference to a private constant.
Automated Tests
Added tests for all features.