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

fix: use tree-sitter-markdown to extract code snippets #1315

Merged
merged 6 commits into from
Feb 21, 2025
Merged

Conversation

blindFS
Copy link
Contributor

@blindFS blindFS commented Feb 20, 2025

Fixes #1288 .

Please review the code with care, and feel free to change it.

I somehow feel there're other unnecessary codes if we choose to implement like this, but I'll leave you to judge.

TODO: new test cases

@yetone
Copy link
Owner

yetone commented Feb 20, 2025

‌‌‌‌‌‌‌Once this PR is merged, does this issue to some extent no longer exist?

#1316

@blindFS
Copy link
Contributor Author

blindFS commented Feb 20, 2025

Which issue do you mean? your link seems to be a PR instead of an issue.

PS. I just found some bugs in current implementation, please wait a sec.

@blindFS
Copy link
Contributor Author

blindFS commented Feb 20, 2025

Oh, you meant PR #1316 somehow fixes #1288 , I haven't tried that yet.

Does that work with cached responses? It's somehow difficult to reproduce that malformed response to test that issue.

@blindFS blindFS marked this pull request as ready for review February 20, 2025 06:13
@blindFS
Copy link
Contributor Author

blindFS commented Feb 20, 2025

I'm a little bit lost in current test framework, I'll leave the task of adding test cases to you if you don't mind.

@blindFS
Copy link
Contributor Author

blindFS commented Feb 21, 2025

@yetone I saw your fix in #1318, theoretically it avoids creating nonsense folders/files. But I still believe tree-sitter based parsing is a more robust way, as it provides a "best guess" when the code block wrappers are not properly matched.
For example, in issue #1288 , this PR will not only stop the unwanted file/folder creation, but also make the last 4 code blocks applicable.

Do you want me to make a similar change to extract_cursor_planning_code_snippets_map ?

@yetone
Copy link
Owner

yetone commented Feb 21, 2025

@yetone I saw your fix in #1318, theoretically it avoids creating nonsense folders/files. But I still believe tree-sitter based parsing is a more robust way, as it provides a "best guess" when the code block wrappers are not properly matched. For example, in issue #1288 , this PR will not only stop the unwanted file/folder creation, but also make the last 4 code blocks applicable.

Do you want me to make a similar change to extract_cursor_planning_code_snippets_map ?

That would be better!

@blindFS blindFS marked this pull request as draft February 21, 2025 04:02
@yetone
Copy link
Owner

yetone commented Feb 21, 2025

Just a reminder, I just changed the start_line and end_line of the codeblock from 0-indexed to 1-indexed in this PR. You also need to make corresponding changes in this PR.

image

https://github.com/yetone/avante.nvim/pull/1335/files#diff-ddcb4139fbc53f068a08051ae32665c76507f66698fe427fcfdd384a36dce2fcL788-R873

@blindFS
Copy link
Contributor Author

blindFS commented Feb 21, 2025

Just a reminder, I just changed the start_line and end_line of the codeblock from 0-indexed to 1-indexed in this PR. You also need to make corresponding changes in this PR.

https://github.com/yetone/avante.nvim/pull/1335/files#diff-ddcb4139fbc53f068a08051ae32665c76507f66698fe427fcfdd384a36dce2fcL788-R873

I've aligned my code to your change, but that change seems to cause new problems for me with planning turned on (another story).

Basically I think I'm done, with extract_cursor_planning_code_snippets_map and parse_codeblocks switched to a tree-sitter-based solution.

And I've tested on my cached malformed response, it works well no matter planning mode is on or off.

@blindFS blindFS marked this pull request as ready for review February 21, 2025 07:55
@yetone
Copy link
Owner

yetone commented Feb 21, 2025

I've aligned my code to your change, but that change seems to cause new problems for me with planning turned on (another story).

Could you please elaborate on this issue?

@blindFS
Copy link
Contributor Author

blindFS commented Feb 21, 2025

I've aligned my code to your change, but that change seems to cause new problems for me with planning turned on (another story).

Could you please elaborate on this issue?

Never mind, it's fixed by your later commits.

@yetone yetone merged commit caa8342 into yetone:main Feb 21, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants