Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork634
refactor(#2988): multi instance change_dir and dir_up#3209
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
alex-courtis left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
This is looking great, really happy with your contributions and it's working as per current behaviour.
Please:
- Remove unused file
- Remove unused variable
- Fix change dir node wrapper
- Change incorrect require
- Move
change_dirto an instance member * - Change inline require to file level
- Remove unnecessary require *
The ones marked with a * will just go away if we move further with this refactor; see incoming comment.
| localM= {} | ||
| ---@paramnodeNode | ||
| functionM.fn(node) |
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.
This function has been moved toExplorer:dir_up and it's working beautifully there.
This is great as this function is now unused, therefore we can delete this whole file.
| ---@returnboolean | ||
| localfunctionshould_change_dir() | ||
| localexplorer=core.get_explorer() |
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.
This variableexplorer is unused and has been identified by the linter:https://github.com/nvim-tree/nvim-tree.lua/actions/runs/18935214550/job/54313031114?pr=3209
This line should be deleted.
| Api.tree.change_root_to_parent=wrap_node(actions.root.dir_up.fn) | ||
| Api.tree.change_root_to_parent=wrap_node(Explorer.dir_up) |
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.
This change calls the newdir_up method. This isn't correct as the call is to the functionExplorer.dir_up rather than the methodExplorer:dir_up.
You can use
wrap_node(wrap_explorer("dir_up"))
instead.wrap_explorer will calldir_up as a method andwrap_node will pass the node.
Why is that a problem? Whendir_up is called,self is set to the node, notexplorer.
You can see this by adding some debug to the start ofExplorer:dir_up
require("nvim-tree.log").line("dev","self %s",selfandself.absolute_pathor"no self")require("nvim-tree.log").line("dev","node %s",nodeandnode.absolute_pathor"no node")
- on the data directory as per base test cases.
[2025-11-03 11:34:32] [dev] self /home/alex/src/keyd/data[2025-11-03 11:34:32] [dev] node no nodeExpected:
[2025-11-03 11:32:34] [dev] self /home/alex/src/keyd[2025-11-03 11:32:34] [dev] node /home/alex/src/keyd/data| localfunctionhandle_buf_cwd(cwd) | ||
| ifM.respect_buf_cwdandcwd~=core.get_cwd()then | ||
| require("nvim-tree.actions.root.change-dir").fn(cwd) | ||
| require("lua.nvim-tree.explorer.change-dir").fn(cwd) |
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.
This change requires the movedchange-dir.lua module. It's throwing an exception as the module does not exist.
The require should berequire("nvim-tree.explorer.change-dir")
See the exception in section "Fail: respect_buf_cwd = true" in the test document.
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.
Great work! Unnecessary code gone!
| returnself:clone() | ||
| end | ||
| ---@paramnodeNode |
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.
Annotations are necessary for linting and LSP integration. Thank you for adding them.
| end | ||
| end | ||
| Explorer.change_dir=change_dir |
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.
Explorer.change_dir is assigned to theExplorer class, instead of the instance. This isn't great as allExplorer instances will share the samechange_dir.
It should be set to the instance during the constructor e.g.self.change_dir = ...
| end | ||
| localnewdir=vim.fn.fnamemodify(utils.path_remove_trailing(cwd),":h") | ||
| require("nvim-tree.explorer.change-dir").fn(newdir) | ||
| require("nvim-tree.actions.finders.find-file").fn(node.absolute_path) |
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.
The modulefind-file is required inline instead of at the start of the module. This isn't desirable: all modules should be required only once at startup and shared. Inline requires can result in problems appearing at runtime rather than startup, as they are lazily evaluated.
localfind_file=require("nvim-tree.actions.finders.find-file")
Why do we have some inline requires? There are circular dependencies that forced it. One of the goals of this refactoring work is to remove circular dependencies.
| return | ||
| end | ||
| localnewdir=vim.fn.fnamemodify(utils.path_remove_trailing(cwd),":h") | ||
| require("nvim-tree.explorer.change-dir").fn(newdir) |
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.
Thechange-dir.lua module is required inline however we can use thechange_dir member ofExplorer. It could be simplified to:
M.change_dir.fn(newdir)
alex-courtis commentedNov 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Testing with Success: Base CasesOpen
Fail: |
This is great, I'm happy to merge (following fixes) OR we can go further in this pull request OR go further in the next pull request. You've refactored the If you're keen we can do the same for ALL the What's the advantage? The code will simplify greatly when they are part of the explorer class:
|
change_dir anddir_up to theExplorer classFYI you canexecute all the CI checks locally so that you don't have to wait for CI. Once you've merged this PR I can add you as a repo member so that you can run CI checks yourself. |
@alex-courtis hope you doing well, thanks for your time, I will be making this changes by saturday and also following all other instructions and comments you left. |
There's no rush; we can take all the time we want. |
Uh oh!
There was an error while loading.Please reload this page.
fixes#2988
This PR is part of one of the Multi Instance tasks and aims to make a refactor moving the root actions
change_diranddir_upto theExplorerclass.PS: This is my first contribution to some nvim plugin, I've using nvim for almost 4-5 months now and become interested in getting involved into its Open source world.