-
Notifications
You must be signed in to change notification settings - Fork 174
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
Replace IndexablePath with ResourceUri concept #2423
Conversation
5c6b0eb
to
624ddc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still prefer a clearer class name than Uri
given its origin is the indexer and not just the language server. But we can worry about it later as well.
BTW, I think this counts as a breaking change as addon tests can use |
624ddc6
to
4e9e819
Compare
BTW, are we still going to need |
We'll get rid of that on the follow up to this PR, where we should start using the new Uri class everywhere. |
4e9e819
to
74b63ab
Compare
Motivation
I spend some time thinking in more detail about #2341. At the core of the issue, I think we're missing our own URI abstraction.
And indexable path is currently the same thing as a file URI, with the extra attribute of require path. If we introduce a Uri concept to the indexer, we can standardize the entire language server and index on it, which will make it more consistent and allow #2341 to move forward in a better way.
In fact, I think we may not even need to store locations in entries anymore, since we only use those to construct URIs, but that can be encoded as the URI's fragment, which I believe may also simplify #2051.
This change has no impact in indexing time or memory used.
Implementation
Created a
RubyIndexer::Uri
object, which is essentially justURI::Generic
with our extra bits of behaviour. The next steps is to continue standardizing onRubyIndexer::Uri
and get rid of other cases ofURI::Generic
.Automated Tests
Adapted existing tests.