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

Replace IndexablePath with ResourceUri concept #2423

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Aug 9, 2024

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 just URI::Generic with our extra bits of behaviour. The next steps is to continue standardizing on RubyIndexer::Uri and get rid of other cases of URI::Generic.

Automated Tests

Adapted existing tests.

@vinistock vinistock added server This pull request should be included in the server gem's release notes other Changes that aren't bugfixes, enhancements or breaking changes labels Aug 9, 2024
@vinistock vinistock self-assigned this Aug 9, 2024
@vinistock vinistock requested a review from a team as a code owner August 9, 2024 18:24
@vinistock vinistock requested review from andyw8 and st0012 August 9, 2024 18:24
@vinistock vinistock requested a review from andyw8 August 9, 2024 21:10
lib/ruby_indexer/lib/ruby_indexer/uri.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/uri.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/uri.rb Outdated Show resolved Hide resolved
lib/ruby_indexer/lib/ruby_indexer/uri.rb Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs-introduce-indexer-uri branch from 5c6b0eb to 624ddc6 Compare August 19, 2024 20:15
@vinistock vinistock requested a review from st0012 August 19, 2024 20:17
Copy link
Member

@st0012 st0012 left a 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.

@st0012
Copy link
Member

st0012 commented Aug 19, 2024

BTW, I think this counts as a breaking change as addon tests can use index_single, which now requires a different type.

@vinistock vinistock force-pushed the vs-introduce-indexer-uri branch from 624ddc6 to 4e9e819 Compare September 3, 2024 17:44
@st0012
Copy link
Member

st0012 commented Sep 3, 2024

BTW, are we still going to need lib/core_ext/uri.rb after this?

@vinistock
Copy link
Member Author

BTW, are we still going to need lib/core_ext/uri.rb after this?

We'll get rid of that on the follow up to this PR, where we should start using the new Uri class everywhere.

@vinistock vinistock force-pushed the vs-introduce-indexer-uri branch from 4e9e819 to 74b63ab Compare September 3, 2024 18:24
@vinistock vinistock changed the title Replace IndexablePath with Uri concept Replace IndexablePath with ResourceUri concept Sep 3, 2024
@vinistock vinistock enabled auto-merge (squash) September 3, 2024 18:30
@vinistock vinistock merged commit 7572fde into main Sep 3, 2024
36 checks passed
@vinistock vinistock deleted the vs-introduce-indexer-uri branch September 3, 2024 18:41
vinistock added a commit that referenced this pull request Sep 4, 2024
vinistock added a commit that referenced this pull request Sep 4, 2024
Revert "Replace IndexablePath with ResourceUri concept (#2423)"

This reverts commit 7572fde.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
other Changes that aren't bugfixes, enhancements or breaking changes 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.

3 participants