-
Notifications
You must be signed in to change notification settings - Fork 154
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
Index files as they are modified #2342
Index files as they are modified #2342
Conversation
0d47a2d
to
ce52fcd
Compare
ae415aa
to
d619b2c
Compare
uri = indexable.to_uri.to_s | ||
original_entries = @uris_to_entries[uri] | ||
sig { params(uri: URI::Generic, indexable: IndexablePath).void } | ||
def handle_change(uri, indexable) |
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.
Are there cases where the uri
would be different than indexable.to_uri
? If there are, can we capture them in test cases?
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.
Also perhaps we can swap the arguments order, and have uri
default to indexable.uri
?
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.
Yeah, I think we need to change IndexablePath
to be simply an Indexable
and then include the URI there. I was just trying to avoid parsing the same URI twice, but the code is ugly.
Do we need to consider the case where someone saves a file outside of the current workspace? |
I don't believe so. If the document is not opened in the editor and it's outside of the current workspace, how would we know that it is relevant for the current project? Or in other words, how could we know which files to watch (we wouldn't want to watch all Ruby files in the computer)? Note I'm afraid that this PR is going to degrade performance very significantly. I'm going to re-think this and #2341 to ensure that we're not making the experience worse. |
Here's a reproduction of what I'm thinking of:
|
I don't think we need to support that use case. Under what circumstances would someone want to do that? You can't even require that file without the full file path because Desktop is not going to be a part of your |
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 know changes are planned, just marking as reviewed so GitHub stops pinging me).
Closing this for now. We need to re-think the approach. |
Motivation
Closes #1908
Index files as they are modified rather than waiting until they are saved. This better reflects the state of the codebase as the user is coding.
Implementation
The idea is the following:
The only extra thing is that we need to be able to handle ancestor changes even if the user hasn't saved the file, so I created a way to track those.
Automated Tests
Added some tests to ensure that we index declarations in unsaved files as they are being modified and also to ensure that we don't accidentally create duplicate entries, which is a risk if we accidentally forget to clean things up.
Manual Tests