-
-
Notifications
You must be signed in to change notification settings - Fork 634
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 our terms of service and privacy 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?
Conversation
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.
| local M = {} | ||
|
|
||
| ---@param node Node | ||
| function M.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 to Explorer:dir_up and it's working beautifully there.
This is great as this function is now unused, therefore we can delete this whole file.
|
|
||
| ---@return boolean | ||
| local function should_change_dir() | ||
| local explorer = 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 variable explorer 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 new dir_up method. This isn't correct as the call is to the function Explorer.dir_up rather than the method Explorer:dir_up.
You can use
wrap_node(wrap_explorer("dir_up"))instead. wrap_explorer will call dir_up as a method and wrap_node will pass the node.
Why is that a problem? When dir_up is called, self is set to the node, not explorer.
You can see this by adding some debug to the start of Explorer:dir_up
require("nvim-tree.log").line("dev", "self %s", self and self.absolute_path or "no self")
require("nvim-tree.log").line("dev", "node %s", node and node.absolute_path or "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 node
Expected:
[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
| local function handle_buf_cwd(cwd) | ||
| if M.respect_buf_cwd and cwd ~= 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 moved change-dir.lua module. It's throwing an exception as the module does not exist.
The require should be require("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!
| return self:clone() | ||
| end | ||
|
|
||
| ---@param node 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.
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 the Explorer class, instead of the instance. This isn't great as all Explorer instances will share the same change_dir.
It should be set to the instance during the constructor e.g. self.change_dir = ...
| end | ||
| local newdir = 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 module find-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.
local find_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 | ||
| local newdir = 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.
The change-dir.lua module is required inline however we can use the change_dir member of Explorer. It could be simplified to:
M.change_dir.fn(newdir)|
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 and dir_up to the Explorer class|
FYI you can execute 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. |
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.