Skip to content

refactor(create_file): use vim.uv functions #1642

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jinzhongjia
Copy link
Contributor

@jinzhongjia jinzhongjia commented Jun 15, 2025

Description

  • replace plenary.path with native vim.uv functions for file and directory operations

Plenary creates files on Windows often fail to create parent successfully.

Using vim's built-in method is more convenient

Related Issue(s)

Checklist

  • I've read the contributing guidelines and have adhered to them in this PR
  • I've updated CodeCompanion.has in the init.lua file for my new feature
  • I've added test coverage for this fix/feature
  • I've updated the README and/or relevant docs pages
  • I've run make all to ensure docs are generated, tests pass and my formatting is applied

- replace plenary.path with native vim functions for file and directory operations
- update file existence and directory checks to use vim.fn
- handle parent directory creation and file writing with vim.fn methods
- add empty string to expected output to match file with trailing newline
@jinzhongjia
Copy link
Contributor Author

The related disscussion: #1641

@jinzhongjia
Copy link
Contributor Author

Actually, I tried to fix the plenary problem, but honestly, the plenary's code is a bit hard to maintain

@jinzhongjia jinzhongjia changed the title refactor(create_file): remove plenary dependency and use vim functions refactor(create_file): use vim functions Jun 15, 2025
@olimorris
Copy link
Owner

Plenary creates files on Windows often fail to create parent successfully.

This is the first I've heard of this and we've had this functionality in the plugin for over a year now. Can you cross-reference the Plenary issue that exists for this.

Plenary is deeply embedded in the plugin so I'm surprised this is the one area you've been experiencing problems with.

@jinzhongjia
Copy link
Contributor Author

Reproduction is easy, just ask codecompanion to use @files to create a non-existent file (the parent folder does not exist either)

like: luas/test.txt

@olimorris
Copy link
Owner

Reproduction is easy, just ask codecompanion to use

I don't have a Windows machine so reproduction isn't easy at all.

This needs to be a bug report filed with Plenary so someone can fix this upstream.

The changes in this PR are minimal but I'm unwilling to change core functionality because a user says they're having a problem without providing any evidence or willing to fix it for the wider community. As I mentioned before, this is the first time I've had a windows issue raised with this functionality.

@jinzhongjia
Copy link
Contributor Author

The main reason is that there is a problem with the parsing parent function of plenary. I see that it uses regular matching
For example, this path C:/Users/jin/code/LspUI.nvim/luas/test.txt will give C:\ after passing parent

@jinzhongjia
Copy link
Contributor Author

jinzhongjia commented Jun 15, 2025

Unfortunately, plenary doesn't seem to be developing actively at the moment, and it uses more ancient methods, such as using vim.loop

In fact, throughout 2024, Plenary has few submissions, and many PRs have been unaudited and not merged.

And this is plenary releated issue: nvim-lua/plenary.nvim#393

Despite claims to be repaired, similar issues remain

@jinzhongjia
Copy link
Contributor Author

I'll change this pr to use uv later

- replace vim.fn with vim.uv for file and directory checks
- add recursive directory creation using uv
- improve error handling for file creation and writing
- ensure file descriptors are properly closed
- remove trailing empty string from expected output
- ensure test matches actual file content
@jinzhongjia jinzhongjia changed the title refactor(create_file): use vim functions refactor(create_file): use vim.uv functions Jun 15, 2025
@jinzhongjia
Copy link
Contributor Author

Now use vim.uv and it can ensure that the function is the same as it was originally

Copy link
Owner

@olimorris olimorris left a comment

Choose a reason for hiding this comment

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

I accept this is a Plenary issue. However, for this to be added into CodeCompanion I want this to exist as a utility file - something that we can reuse across the plugin. It's no use existing in one tool if this issue is going to arise again, elsewhere in the plugin, in the future.

The impact of this is we will need test coverage for that utility file as well.

So unfortunately there's a bit of work to get this PR in a state that I'd be happy to accept.

@jinzhongjia
Copy link
Contributor Author

OK, then according to my understanding, what I need to do next is

  • move create_dir_recursive to utils/files.lua and add unit test
  • The return value of the create does not follow the convention

@jinzhongjia jinzhongjia marked this pull request as draft June 15, 2025 11:47
- implement files.create_dir_recursive for robust directory creation
- refactor create_file tool to use new utility and improve error handling
- add comprehensive tests for create_dir_recursive, including edge cases and logging
- move require("codecompanion.utils.log") after files import
- adjust require order in test_files.lua for readability
@jinzhongjia jinzhongjia marked this pull request as ready for review June 15, 2025 14:58
@jinzhongjia
Copy link
Contributor Author

Maybe now the requirement is met

@jinzhongjia jinzhongjia requested a review from olimorris June 15, 2025 15:15
@jinzhongjia jinzhongjia marked this pull request as draft June 18, 2025 08:56
- replace describe/it blocks with MiniTest.new_set structure
- update test hooks to pre_case and post_case
- migrate all create_dir_recursive tests to new MiniTest style
- improve Windows root directory test with MiniTest.skip
- update log.error mocks for compatibility with new test style
- return test set at end of file
- remove nested pcall and use direct error checks
- provide clearer error messages for directory, file open, write, and close failures
- combine write and close errors for comprehensive reporting
- streamline control flow for better readability
@jinzhongjia jinzhongjia marked this pull request as ready for review June 18, 2025 17:07
@jinzhongjia
Copy link
Contributor Author

I've corrected the code according to the specification

I carefully checked the specifications of other codes and the implementation is really great

Thank you for your work

@jinzhongjia
Copy link
Contributor Author

jinzhongjia commented Jun 21, 2025

I also tried to run unit tests on Windows, but found that there were basically no errors. The code specifications were written really well. The problem was that there were some problems with the backslash processing, but it did not affect the use, but it would only cause some unit tests to be unable to pass on Windows

- add vim.fs.normalize to ensure consistent file paths
- prevent potential issues with unnormalized paths
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.

2 participants