-
Notifications
You must be signed in to change notification settings - Fork 135
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
Use a custom URI class to handle source location comments #1094
Use a custom URI class to handle source location comments #1094
Conversation
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 think it may be too much effort but are we able to have a test for the BUNDLER_HOME
scenario?
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 am not sure if I understand why we are doing more breakdowns here. Isn't it the case that if I have foo-1.2.3
as a dependency, that it should be referenced in the RBI file as foo-1.2.3
? Do we care if I suddenly change my Bundler setup so that it installs foo
to the bundle path? If so, why do we care?
@paracycle while testing this on Core, I realized that we have a mix of both. Some gems are installed under If this information is not present somehow in the source location comments, the Ruby LSP has no way of figuring out where the appropriate path is. I have no clue why we have gems on both locations though. If you have more context on why this could be, let me know. |
LSP can search in both locations? |
Is that better though? That means that instead of processing that ahead of time in Tapioca, we'll need to search on both when serving the request. |
Yes, I think it is better. IMO, this mechanism should be open to the gem moving from one valid place to another, and as long as the gem is the same version, we should still be able to locate it without an error. So, encoding a lot of information on the generation side is not the best way to go, the client should be able to look things up on consumption side. After all, that's exactly what happens with Ruby tools, they all resolve a gem reference by looking it up in various place the gem could be. |
798da50
to
363b9fc
Compare
f8272bd
to
0da60d8
Compare
5fcf582
to
183baf6
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.
This is looking really good but I still have a few comments.
d3d045c
to
a689279
Compare
Code looks good to me. Questions for my own understanding:
|
@dirceu Yeah, for now the Ruby LSP is the only consumer of the source location comments. However, Sorbet could also consume them to include them as a location on go to definition. What do you mean by multiple formats? |
@vinistock I meant |
@dirceu yes, we changed the format to |
b57b4f3
to
eca75c5
Compare
- Create a custom URI::Source handler for source location comments - Better handle default gems
eca75c5
to
539c687
Compare
Motivation
We are currently relying on regexes and string substitutions to find the right paths for gems. This is not reliable and has proven to not find the right paths.
Implementation
This PR introduces a custom URI object to handle the
source://
scheme. This makes it easier to validate the consistency of the URI and we can use a similar object on the Ruby LSP to do the inverse operation of parsing it.It also improves the detection of default gem paths, by using
Gemfile::GemSpec
, which already has the mechanisms to figure out these paths for us.The format of source URIs is now:
Tests
Updated existing tests.