Skip to content

feat: go-to-definition, fix bugs, update deps, remove unused files #635

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

Draft
wants to merge 51 commits into
base: develop
Choose a base branch
from

Conversation

aldur
Copy link

@aldur aldur commented Jul 10, 2025

I am opening this PR as draft since there is a big intersection with #634 and it probably does too many things at once. I have also cherry-picked a couple commits that have since been pushed to develop. I have rebased it on develop so that it should include all changes.

If interesting we can split it into multiple PRs; the intent is to kickstart the conversation here.
I am opening it against develop and leaving review comments inline to explain the rationale behind the changes.

For context, I have been using this project for a while, thank you for creating it!

This PR:

  1. Implements LSP go-to-definition for accounts and commodities.
  2. Fixes bugs related to references and renames, while making them re-use existing code (for instance, using the code to fetch references and then renaming them, instead of duplicating it).
  3. Turns default log-level to INFO (this was making it hard for nvim to cope when editing a big Beancount file).
  4. Adds some more testing and extracts the testing utils to a different file, so they can be re-used across tests / modules.
  5. Updates to the latest tree-sitter version supported by the grammar, which introduced some light breaking changes.
  6. Where possible, uses idiomatic Rust e.g. to create collections from iterators.
  7. Unifies the naming of providers (some started with handle_, others did not).
  8. Updates the README with the latest developments.
  9. Removes a ton of files that, to the best of what I could see, are unused?

I have tested my changes against nvim. I might have obviously missed things, but have tried to add a few tests to increase confidence.

eastonman and others added 30 commits March 8, 2025 00:01
the project version is incorrect
also remove some likely ai generated comments
also remove systemd on darwin.
this works on my apple sillicon macbook
…pport-relative-path

feat: support relative path in journal_root
chore: include LICENSE in the packaged crate
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
}

#[test]
fn handle_account_completion_case_sensitive() {
Copy link
Author

Choose a reason for hiding this comment

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

I had previously added this test, which is not probably redundant with handle_case_insensitive_completion.

Comment on lines +21 to +22
let node =
lsp_position_to_node(&doc.content, params.text_document_position.position, tree).unwrap();
Copy link
Author

Choose a reason for hiding this comment

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

Using this instead of the logic that was previously there because otherwise references wouldn't work with the cursor on the first letter of an account (maybe an off-by-one error).

let url = url::Url::from_str(self.as_str()).map_err(|_| ())?;
tracing::info!("TOFILEPATH {:#?}", url);
tracing::debug!("TOFILEPATH {:#?}", url);
Copy link
Author

Choose a reason for hiding this comment

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

this was needlessly verbose I think

@@ -8,132 +8,103 @@
url = "github:oxalica/rust-overlay";
inputs.nixpkgs.follows = "nixpkgs";
};
crane = {
url = "github:ipetkov/crane";
inputs.nixpkgs.follows = "nixpkgs";
Copy link
Author

Choose a reason for hiding this comment

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

input doesn't exist anymore

@@ -150,18 +121,10 @@
git-cliff
virt-viewer
];
LD_LIBRARY_PATH = pkgs.lib.makeLibraryPath [
Copy link
Author

Choose a reason for hiding this comment

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

I don't know why this was set, but it was breaking my nvim setup.

@@ -8,132 +8,103 @@
url = "github:oxalica/rust-overlay";
inputs.nixpkgs.follows = "nixpkgs";
};
crane = {
Copy link
Author

Choose a reason for hiding this comment

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

sorry about all the whitespace noise here!

@polarmutex
Copy link
Owner

I will look at this as soon as I can

@polarmutex
Copy link
Owner

at quick glance it looks good. I am going to merge and test out when i do my financial updates

@polarmutex
Copy link
Owner

can not merge into development , going to cherry pick into development and and lets create issues/discussions on further improvements

@polarmutex
Copy link
Owner

git history is hard to merge. i prefer rebase to merge commits. I will fix the small issues. put the following features in their own pr and have it merge into main

  • goto account
  • goto commodity
  • anything else

@polarmutex polarmutex force-pushed the develop branch 11 times, most recently from e008224 to efaabfd Compare July 12, 2025 04:49
@aldur
Copy link
Author

aldur commented Jul 12, 2025

Sounds good!

@polarmutex polarmutex force-pushed the develop branch 3 times, most recently from 6ef2b68 to 021e143 Compare July 12, 2025 22:32
@polarmutex polarmutex force-pushed the develop branch 2 times, most recently from 97aada2 to f249e00 Compare August 2, 2025 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants