plugins icon indicating copy to clipboard operation
plugins copied to clipboard

feat(git): improve readability and maintainability

Open PFiS1737 opened this issue 10 months ago • 11 comments

Description

  • Improve readability and maintainability.
  • Default usage of nf-oct-diff series icons from Nerdfonts and terminal colors (instead of RGB colors).

PFiS1737 avatar Mar 02 '25 07:03 PFiS1737

This PR contains too much content, which makes it difficult to diff. Is it possible to split it into several smaller PRs?

sxyazi avatar Mar 02 '25 13:03 sxyazi

Sorry, but I'm afraid this would be quite difficult, as it involves an almost complete refactor with various parts intertwined. However, I can provide some technical details:

  • First, I removed propagate_down and the status marked as 30, which was very difficult to understand without comments. To restore the retrieval of the ignored status related to this, I wrote the get_kind function. This function searches upwards layer by layer and makes the file (or folder) ignored if its parent folder is marked as ignored.
  • Secondly, to facilitate maintenance and improve code readability, I introduced the Status enum along with a series of type annotations.
  • Thirdly, I added the detection of the renamed status, mainly involving lines 113-116, which are easy to understand.

I have no classes tomorrow afternoon, so I can try to split it into multiple commits. However, it might be a bit late if you're already working on reviewing this PR :(

PFiS1737 avatar Mar 02 '25 14:03 PFiS1737

If you don't mind, I will force push to the PR after I finish. The new commits include the following changes:

  • Reverted the configuration options for icons and styles in setup (but I have kept the related commits locally and am ready to patch them anytime — trust me, people tend to prefer keeping configurations together).
  • Used THEME consistently in both the README and main.lua, as I still couldn't get different variables to work together (THEME in init.lua and th in main.lua) — as I mentioned above. (with the latest commit 38d5b2f)
  • Unified variable names.
  • Added caching in get_status (previously get_kind) to improve performance.

PFiS1737 avatar Mar 03 '25 05:03 PFiS1737

Sorry, I didn't realize that. I will try to revert it. (Perhaps there's a better way to handle this? The introduction of 30 has increased the complexity and difficulty of understanding the code.)

PFiS1737 avatar Mar 03 '25 07:03 PFiS1737

IMO this PR should just focus on adding the new feature and keep the changes as small as possible so it’s easier to review. Improving code readability is great, but it might be worth doing that in a separate PR where we can dig into it a bit more and discuss it further.

On Mon, Mar 3, 2025 at 3:04 PM PFiS @.***> wrote:

Sorry, I didn't realize that. I will try to revert it. (Perhaps there's a better way to handle this? The introduction of 30 has increased the complexity and difficulty of understanding the code.)

— Reply to this email directly, view it on GitHub https://github.com/yazi-rs/plugins/pull/78#issuecomment-2693457450, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEFWFIHLZSUJZ5BOL6EAUCL2SP5GZAVCNFSM6AAAAABYE6ER3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJTGQ2TONBVGA . You are receiving this because you commented.Message ID: @.***> [image: PFiS1737]PFiS1737 left a comment (yazi-rs/plugins#78) https://github.com/yazi-rs/plugins/pull/78#issuecomment-2693457450

Sorry, I didn't realize that. I will try to revert it. (Perhaps there's a better way to handle this? The introduction of 30 has increased the complexity and difficulty of understanding the code.)

— Reply to this email directly, view it on GitHub https://github.com/yazi-rs/plugins/pull/78#issuecomment-2693457450, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEFWFIHLZSUJZ5BOL6EAUCL2SP5GZAVCNFSM6AAAAABYE6ER3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJTGQ2TONBVGA . You are receiving this because you commented.Message ID: @.***>

sxyazi avatar Mar 03 '25 07:03 sxyazi

Sorry for wasting your time, and please forgive my unfamiliarity. I will revert the commits introducing the new feature, making this PR solely focused on discussing improvements to code readability and maintainability.

PFiS1737 avatar Mar 03 '25 07:03 PFiS1737

The rollback is complete, and I have translated the original logic according to the latest type definitions. I will continue to explore how to remove the __ignored and "" entries in dirs_to_repo, which are confusing. Or I will try adding comments.

PFiS1737 avatar Mar 03 '25 08:03 PFiS1737

Well, I failed — I couldn't come up with any method other than using an additional flag.

However, I made some optimizations:

  • remove the conditional checks in bubble_up and propagate_down since they always pass
  • add comments and use named flags to make the code a bit easier to understand
  • and some minor adjustments

PFiS1737 avatar Mar 03 '25 13:03 PFiS1737

I have only reviewed part of it, and it is still very difficult to review as the different changes are intertwined and this PR refactored almost the entire plugin, and I am not sure how to correctly review them.

I'll try to split these changes into several smaller PRs later and request you review them. I will add you as a collaborator to make sure you get the credits.

sxyazi avatar Mar 04 '25 05:03 sxyazi

Sincerely appreciate your efforts and thank you for your patience these days. I will do my best to fulfill my part.

PFiS1737 avatar Mar 04 '25 06:03 PFiS1737

Hi, I'm planning to submit separate PRs over the next few days to complete the updates:

  • [x] Simplify the code — mainly by removing unnecessary logic, possibly with some variable name changes. (#80, #81, #82)
  • [x] Adjust the priority of codes (untracked higher, perhaps needs discussion?). (#83)
  • [x] Add ~~type declarations and~~ some comments. (#84)
  • [x] Modify the default styles and signs to use terminal colors and consistent series icons. (#86)
  • [ ] Add support for renamed status. (#88)
  • [ ] Add highlight support for list entries.

Today's will be the first.

You can review them and give feedback whenever you have time, and I'll submit the next one once the previous one is merged.

PFiS1737 avatar Mar 06 '25 05:03 PFiS1737