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

refactor: Decouple walker & indexing logic #981

Merged
merged 3 commits into from
Jul 1, 2022
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Jun 30, 2022

Closes #881

This refactoring solves two problems:

  • reduce dependencies of the walker and make it focused purely on walking by moving out the indexing logic
  • move all indexing logic to a new dedicated indexer package, so that it's easier deduplicate and reuse code within the package

I moved most of the indexing logic pretty much as-is to avoid more noise in already big PR. I assume that we can do more factoring later in a separate PR.

I did consider adding tests to the new indexer package, but then decided against it until it's refactored, and also because it's already being tested anyway as part of didOpen, didChange, didChangeWatchedFiles handlers and walker package.

@radeksimko radeksimko force-pushed the f-walker-refactoring branch 2 times, most recently from e75a715 to ac1ba77 Compare June 30, 2022 10:37
@radeksimko radeksimko added this to the v0.29.0 milestone Jun 30, 2022
@radeksimko radeksimko marked this pull request as ready for review June 30, 2022 10:53
@radeksimko radeksimko requested a review from a team as a code owner June 30, 2022 10:53
This refactoring solves two problems:
 - reduce dependencies of the walker and make it focused on _walking_ by moving out the indexing logic
 - move all indexing logic to a new dedicated indexer package, so that it’s easier deduplicate and reuse code within the package
@radeksimko radeksimko force-pushed the f-walker-refactoring branch from b4d386b to 11c70d8 Compare July 1, 2022 11:36
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring! I've reviewed the first and last commit and tested everything locally.

Everything works fine so far 👍

@radeksimko radeksimko merged commit 8fba5f0 into main Jul 1, 2022
@radeksimko radeksimko deleted the f-walker-refactoring branch July 1, 2022 13:32
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

walker: Decouple WalkFunc to reduce walker dependencies
2 participants