-
-
Notifications
You must be signed in to change notification settings - Fork 638
refactor(#2988): multi instance dir_up #3222
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
refactor(#2988): multi instance dir_up #3222
Conversation
|
Thanks @Uanela I will get to a review on the weekend. What sort of task would you like to work on next? Another refactor? Bug? Feature? |
|
I guess a another refactor would help me still getting along with the codebase. |
Base Cases OKOpen
|
alex-courtis
left a comment
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.
Looking good @Uanela , all tests pass and it's working beautifully.
Only one small change please:
lua/nvim-tree/explorer/init.lua
Outdated
|
|
||
| local newdir = vim.fn.fnamemodify(utils.path_remove_trailing(cwd), ":h") | ||
| require("nvim-tree.actions.root.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.
Please move these requires up to the top of the file. Here's an example you can follow: https://github.com/nvim-tree/nvim-tree.lua/blob/master/lua/nvim-tree/actions/tree/open.lua#L3
Why do this at the start of the file?
- We only want to do it once, for speed
- Any problems with the require are found immediately at startup, instead of later at runtime
I'll get back to you shortly. The issue backlog needs to be groomed and tidied. |
|
Got you Mr |
|
Changes made. Lately I've been kind of understanding why some of my coworkers just avoid collaborating on a internal tool of the company I work, I started alone and for new one to come up can be a little bit daunting (not that the come is mess) But simply because you gotta get comfortable around a codebase that just works, mainly when you are still getting into the software development carrer And I am just feeling the same here, is kind of I am just learning to write code from the beginning, I gotta get along with a new language (even though not hard) and a codebase that just works, have been interesting, once again @alex-courtis thanks for the onboarding. I've been also creating some plugins (even if they exist) just for practicing. I will get hands on with the change dir refactor and with the new issues you just mentioned. |
alex-courtis
left a comment
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.
All test cases passing, looks good, love your work @Uanela
Congrats on your first merge!
|
Well done! 👍 |
Refactored the dir-up action by moving it from a standalone module into the Explorer class as a method. This improves code organization by consolidating explorer-related functionality within the Explorer class itself.
Changes